fix: Reply time calculation for re-opened conversations (#11787)
This PR fixes the reply time calculation for reopened conversations. Previously, when a customer sent a message to reopen a resolved conversation, the reply time metric would be calculated incorrectly because the `waiting_since` timestamp was not properly set before the reply event was dispatched. This would create a case where you'd have reporting events like the following ``` [[33955732, "reply_time", 19.0], [33955847, "reply_time", 24.0], [33955666, "reply_time", 89.0], [33955530, "conversation_bot_handoff", 4.0], [33955567, "first_response", 42.0], [33955745, "reply_time", 21.0], [33955934, "reply_time", 49.0], [33955906, "reply_time", 121.0], [33987938, "conversation_resolved", 26285.0], [35571005, "reply_time", 985492.0]] ``` Note the `reply_time` after `conversation_resolved` The fix ensures that `waiting_since` is correctly updated when conversations are reopened, either through incoming messages or manual status changes, resulting in accurate reply time metrics that measure only the time from the customer's new message to the agent's response. ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? The changes have been tested with comprehensive specs that verify: 1. **Reply time calculation after conversation reopening** - Ensures correct timestamps are used when calculating reply times for reopened conversations 2. **Waiting since updates on status changes** - Verifies that `waiting_since` is properly set when conversation status changes from resolved to open 3. **Test the happy path** - Happy path is tested to ensure the `reply_time` and `first_response_time` is correctly calculated Test instructions: 1. Create a conversation with the last message from a customer and resolve it 2. Have an agent reopen it and reply to it 4. When an agent replies, verify that the agent reply_time event is not created for this message To fix any existing data, I've written a small script: https://gist.github.com/scmmishra/fdf458863f2d971978327bbfd5232d0c --------- Co-authored-by: Muhsin Keloth <muhsinkeramam@gmail.com>
This commit is contained in:
@@ -47,6 +47,10 @@ class ReportingEventListener < BaseListener
|
||||
message = extract_message_and_account(event)[0]
|
||||
conversation = message.conversation
|
||||
waiting_since = event.data[:waiting_since]
|
||||
|
||||
return if waiting_since.blank?
|
||||
|
||||
# When waiting_since is nil, set reply_time to 0
|
||||
reply_time = message.created_at.to_i - waiting_since.to_i
|
||||
|
||||
reporting_event = ReportingEvent.new(
|
||||
|
||||
@@ -198,11 +198,21 @@ class Conversation < ApplicationRecord
|
||||
private
|
||||
|
||||
def execute_after_update_commit_callbacks
|
||||
handle_resolved_status_change
|
||||
notify_status_change
|
||||
create_activity
|
||||
notify_conversation_updation
|
||||
end
|
||||
|
||||
def handle_resolved_status_change
|
||||
# When conversation is resolved, clear waiting_since using update_column to avoid callbacks
|
||||
return unless saved_change_to_status? && status == 'resolved'
|
||||
|
||||
# rubocop:disable Rails/SkipsModelValidations
|
||||
update_column(:waiting_since, nil)
|
||||
# rubocop:enable Rails/SkipsModelValidations
|
||||
end
|
||||
|
||||
def ensure_snooze_until_reset
|
||||
self.snoozed_until = nil unless snoozed?
|
||||
end
|
||||
|
||||
@@ -66,6 +66,34 @@ describe ReportingEventListener do
|
||||
end
|
||||
|
||||
describe '#reply_created' do
|
||||
let(:contact) { create(:contact, account: account) }
|
||||
|
||||
def create_customer_message(conversation, created_at: Time.current)
|
||||
create(:message,
|
||||
message_type: 'incoming',
|
||||
account: account,
|
||||
inbox: inbox,
|
||||
conversation: conversation,
|
||||
sender: contact,
|
||||
created_at: created_at)
|
||||
end
|
||||
|
||||
def create_agent_message(conversation, created_at: Time.current, sender: user)
|
||||
create(:message,
|
||||
message_type: 'outgoing',
|
||||
account: account,
|
||||
inbox: inbox,
|
||||
conversation: conversation,
|
||||
sender: sender,
|
||||
created_at: created_at)
|
||||
end
|
||||
|
||||
def create_reply_event(agent_message, waiting_since, event_time = nil)
|
||||
Events::Base.new('reply.created', event_time || agent_message.created_at,
|
||||
waiting_since: waiting_since,
|
||||
message: agent_message)
|
||||
end
|
||||
|
||||
it 'creates reply created event' do
|
||||
event = Events::Base.new('reply.created', Time.zone.now, waiting_since: 2.hours.ago, message: message)
|
||||
listener.reply_created(event)
|
||||
@@ -74,6 +102,88 @@ describe ReportingEventListener do
|
||||
expect(events.length).to be 1
|
||||
expect(events.first.value).to be_within(1).of(7200)
|
||||
end
|
||||
|
||||
context 'when conversation is reopened' do
|
||||
let(:resolved_conversation) do
|
||||
create(:conversation, account: account, inbox: inbox, assignee: user,
|
||||
status: 'resolved', contact: contact)
|
||||
end
|
||||
|
||||
context 'when customer sends message after resolution' do
|
||||
it 'calculates reply time from the reopening message' do
|
||||
customer_message_time = 3.hours.ago
|
||||
create_customer_message(resolved_conversation, created_at: customer_message_time)
|
||||
|
||||
resolved_conversation.reload
|
||||
expect(resolved_conversation.status).to eq('open')
|
||||
|
||||
agent_reply_time = 1.hour.ago
|
||||
agent_message = create_agent_message(resolved_conversation, created_at: agent_reply_time)
|
||||
|
||||
event = create_reply_event(agent_message, customer_message_time)
|
||||
listener.reply_created(event)
|
||||
|
||||
events = account.reporting_events.where(name: 'reply_time', conversation_id: resolved_conversation.id)
|
||||
expect(events.length).to be 1
|
||||
expect(events.first.value).to be_within(60).of(7200)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when conversation has multiple reopenings' do
|
||||
it 'tracks reply time correctly for each reopening' do
|
||||
create_customer_message(resolved_conversation, created_at: 5.hours.ago)
|
||||
first_agent_reply = create_agent_message(resolved_conversation, created_at: 4.hours.ago)
|
||||
|
||||
event = create_reply_event(first_agent_reply, 5.hours.ago)
|
||||
listener.reply_created(event)
|
||||
|
||||
resolved_conversation.update!(status: 'resolved')
|
||||
|
||||
create_customer_message(resolved_conversation, created_at: 2.hours.ago)
|
||||
second_agent_reply = create_agent_message(resolved_conversation, created_at: 1.5.hours.ago)
|
||||
|
||||
event = create_reply_event(second_agent_reply, 2.hours.ago)
|
||||
listener.reply_created(event)
|
||||
|
||||
events = account.reporting_events.where(name: 'reply_time', conversation_id: resolved_conversation.id)
|
||||
.order(created_at: :asc)
|
||||
expect(events.length).to be 2
|
||||
expect(events.first.value).to be_within(60).of(3600)
|
||||
expect(events.second.value).to be_within(60).of(1800)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when conversation is manually reopened' do
|
||||
it 'sets waiting_since when first customer message arrives after manual reopening' do
|
||||
resolved_conversation.update!(status: 'open')
|
||||
|
||||
customer_message_time = 1.hour.ago
|
||||
create_customer_message(resolved_conversation, created_at: customer_message_time)
|
||||
|
||||
agent_reply_time = 15.minutes.ago
|
||||
agent_message = create_agent_message(resolved_conversation, created_at: agent_reply_time)
|
||||
|
||||
event = create_reply_event(agent_message, customer_message_time)
|
||||
listener.reply_created(event)
|
||||
|
||||
events = account.reporting_events.where(name: 'reply_time', conversation_id: resolved_conversation.id)
|
||||
expect(events.length).to be 1
|
||||
expect(events.first.value).to be_within(60).of(2700)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when waiting_since is nil' do
|
||||
it 'does not creates reply time events' do
|
||||
agent_message = create_agent_message(resolved_conversation)
|
||||
|
||||
event = create_reply_event(agent_message, nil)
|
||||
listener.reply_created(event)
|
||||
|
||||
events = account.reporting_events.where(name: 'reply_time', conversation_id: resolved_conversation.id)
|
||||
expect(events.length).to be 0
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '#first_reply_created' do
|
||||
|
||||
@@ -836,4 +836,117 @@ RSpec.describe Conversation do
|
||||
expect(message_window_service).to have_received(:can_reply?)
|
||||
end
|
||||
end
|
||||
|
||||
describe 'reply time calculation flows' do
|
||||
include ActiveJob::TestHelper
|
||||
|
||||
let(:account) { create(:account) }
|
||||
let(:inbox) { create(:inbox, account: account) }
|
||||
let(:contact) { create(:contact, account: account) }
|
||||
let(:agent) { create(:user, account: account, role: :agent) }
|
||||
let(:conversation) { create(:conversation, account: account, inbox: inbox, contact: contact, assignee: agent, waiting_since: nil) }
|
||||
let(:conversation_start_time) { 5.hours.ago }
|
||||
|
||||
before do
|
||||
create(:inbox_member, user: agent, inbox: inbox)
|
||||
# rubocop:disable Rails/SkipsModelValidations
|
||||
conversation.update_column(:waiting_since, nil)
|
||||
conversation.update_column(:created_at, conversation_start_time)
|
||||
# rubocop:enable Rails/SkipsModelValidations
|
||||
conversation.messages.destroy_all
|
||||
conversation.reporting_events.destroy_all
|
||||
conversation.reload
|
||||
end
|
||||
|
||||
def create_customer_message(conversation, created_at: Time.current)
|
||||
message = nil
|
||||
perform_enqueued_jobs do
|
||||
message = create(:message,
|
||||
message_type: 'incoming',
|
||||
account: conversation.account,
|
||||
inbox: conversation.inbox,
|
||||
conversation: conversation,
|
||||
sender: conversation.contact,
|
||||
created_at: created_at)
|
||||
end
|
||||
message
|
||||
end
|
||||
|
||||
def create_agent_message(conversation, created_at: Time.current)
|
||||
message = nil
|
||||
perform_enqueued_jobs do
|
||||
message = create(:message,
|
||||
message_type: 'outgoing',
|
||||
account: conversation.account,
|
||||
inbox: conversation.inbox,
|
||||
conversation: conversation,
|
||||
sender: conversation.assignee,
|
||||
created_at: created_at)
|
||||
end
|
||||
message
|
||||
end
|
||||
|
||||
it 'correctly tracks waiting_since and creates first response time events' do
|
||||
create_customer_message(conversation, created_at: conversation_start_time)
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_within(1.second).of(conversation_start_time)
|
||||
|
||||
# Agent replies - this should create first response event
|
||||
agent_reply1_time = 4.hours.ago
|
||||
create_agent_message(conversation, created_at: agent_reply1_time)
|
||||
|
||||
first_response_events = account.reporting_events.where(name: 'first_response', conversation_id: conversation.id)
|
||||
expect(first_response_events.count).to eq(1)
|
||||
expect(first_response_events.first.value).to be_within(1.second).of(1.hour)
|
||||
|
||||
# the first response should also clear the waiting_since
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_nil
|
||||
end
|
||||
|
||||
it 'does not reset waiting_since if customer sends another message' do
|
||||
create_customer_message(conversation, created_at: conversation_start_time)
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_within(1.second).of(conversation_start_time)
|
||||
|
||||
create_customer_message(conversation, created_at: 3.hours.ago)
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_within(1.second).of(conversation_start_time)
|
||||
end
|
||||
|
||||
it 'records the correct reply_time for subsequent messages' do
|
||||
create_customer_message(conversation, created_at: conversation_start_time)
|
||||
create_agent_message(conversation, created_at: 4.hours.ago)
|
||||
create_customer_message(conversation, created_at: 3.hours.ago)
|
||||
|
||||
create_agent_message(conversation, created_at: 2.hours.ago)
|
||||
reply_events = account.reporting_events.where(name: 'reply_time', conversation_id: conversation.id)
|
||||
expect(reply_events.count).to eq(1)
|
||||
expect(reply_events.first.value).to be_within(1.second).of(1.hour)
|
||||
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_nil
|
||||
end
|
||||
|
||||
it 'records zero reply time if an agent sends a message after resolution' do
|
||||
create_customer_message(conversation, created_at: conversation_start_time)
|
||||
create_agent_message(conversation, created_at: 4.hours.ago)
|
||||
create_customer_message(conversation, created_at: 3.hours.ago)
|
||||
|
||||
conversation.toggle_status
|
||||
expect(conversation.status).to eq('resolved')
|
||||
|
||||
conversation.toggle_status
|
||||
expect(conversation.status).to eq('open')
|
||||
|
||||
conversation.reload
|
||||
expect(conversation.waiting_since).to be_nil
|
||||
|
||||
create_agent_message(conversation, created_at: 1.hour.ago)
|
||||
# update_waiting_since will ensure that no events were created since the waiting_since was nil
|
||||
# if the event is created it should log zero value, we have handled that in the reporting_event_listener
|
||||
reply_events = account.reporting_events.where(name: 'reply_time', conversation_id: conversation.id)
|
||||
expect(reply_events.count).to eq(0)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user