KumiSystems/Chatwoot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

chore: Fix conversation status in webhooks (#3364)

Browse source 
- fix the wrong conversation status being sent in webhooks
- additional information in websocket events
- refactor activity messaging code
- move activity message generation to background job to stop the callback loop
This commit is contained in:
Sojan Jose 2021-11-12 16:17:59 +05:30 committed by GitHub
parent b119d9e729
commit d78cb67a2a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 219 additions and 136 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
app/jobs/conversations/activity_message_job.rb Normal file
View file

@ -0,0 +1,7 @@
class Conversations::ActivityMessageJob < ApplicationJob
queue_as :default
def perform(conversation, message_params)
conversation.messages.create(message_params)
end
end

7
app/listeners/action_cable_listener.rb
View file

@ -143,6 +143,11 @@ class ActionCableListener < BaseListener
def broadcast(account, tokens, event_name, data)
return if tokens.blank?
::ActionCableBroadcastJob.perform_later(tokens.uniq, event_name, data.merge(account_id: account.id))
payload = data.merge(account_id: account.id)
# So the frondend knows who performed the action.
# Useful in cases like conversation assignment for generating a notification with assigner name.
payload[:performer] = Current.user&.push_event_data if Current.user.present?
::ActionCableBroadcastJob.perform_later(tokens.uniq, event_name, payload)
end
end

102
app/models/concerns/activity_message_handler.rb Normal file
View file

@ -0,0 +1,102 @@
module ActivityMessageHandler
extend ActiveSupport::Concern
private
def create_activity
user_name = Current.user.name if Current.user.present?
status_change_activity(user_name) if saved_change_to_status?
create_label_change(user_name) if saved_change_to_label_list?
end
def status_change_activity(user_name)
create_status_change_message(user_name)
queue_conversation_auto_resolution_job if open?
end
def activity_message_params(content)
{ account_id: account_id, inbox_id: inbox_id, message_type: :activity, content: content }
end
def create_status_change_message(user_name)
content = if user_name
I18n.t("conversations.activity.status.#{status}", user_name: user_name)
elsif resolved?
I18n.t('conversations.activity.status.auto_resolved', duration: auto_resolve_duration)
end
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def create_label_added(user_name, labels = [])
return unless labels.size.positive?
params = { user_name: user_name, labels: labels.join(', ') }
content = I18n.t('conversations.activity.labels.added', **params)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def create_label_removed(user_name, labels = [])
return unless labels.size.positive?
params = { user_name: user_name, labels: labels.join(', ') }
content = I18n.t('conversations.activity.labels.removed', **params)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def create_muted_message
return unless Current.user
params = { user_name: Current.user.name }
content = I18n.t('conversations.activity.muted', **params)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def create_unmuted_message
return unless Current.user
params = { user_name: Current.user.name }
content = I18n.t('conversations.activity.unmuted', **params)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def generate_team_change_activity_key
key = team_id ? 'assigned' : 'removed'
key += '_with_assignee' if key == 'assigned' && saved_change_to_assignee_id? && assignee
key
end
def generate_team_name_for_activity
previous_team_id = previous_changes[:team_id][0]
Team.find_by(id: previous_team_id)&.name if previous_team_id.present?
end
def create_team_change_activity(user_name)
return unless user_name
key = generate_team_change_activity_key
params = { assignee_name: assignee&.name, team_name: team&.name, user_name: user_name }
params[:team_name] = generate_team_name_for_activity if key == 'removed'
content = I18n.t("conversations.activity.team.#{key}", **params)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
def generate_assignee_change_activity_content(user_name)
params = { assignee_name: assignee&.name, user_name: user_name }.compact
key = assignee_id ? 'assigned' : 'removed'
key = 'self_assigned' if self_assign? assignee_id
I18n.t("conversations.activity.assignee.#{key}", **params)
end
def create_assignee_change_activity(user_name)
return unless user_name
content = generate_assignee_change_activity_content(user_name)
Conversations::ActivityMessageJob.perform_later(self, activity_message_params(content)) if content
end
end

31
app/models/concerns/assignment_handler.rb
View file

@ -43,35 +43,4 @@ module AssignmentHandler
create_assignee_change_activity(user_name)
end
end
def generate_team_change_activity_key
key = team_id ? 'assigned' : 'removed'
key += '_with_assignee' if key == 'assigned' && saved_change_to_assignee_id? && assignee
key
end
def create_team_change_activity(user_name)
return unless user_name
key = generate_team_change_activity_key
params = { assignee_name: assignee&.name, team_name: team&.name, user_name: user_name }
if key == 'removed'
previous_team_id = previous_changes[:team_id][0]
params[:team_name] = Team.find_by(id: previous_team_id)&.name if previous_team_id.present?
end
content = I18n.t("conversations.activity.team.#{key}", **params)
messages.create(activity_message_params(content))
end
def create_assignee_change_activity(user_name)
return unless user_name
params = { assignee_name: assignee&.name, user_name: user_name }.compact
key = assignee_id ? 'assigned' : 'removed'
key = 'self_assigned' if self_assign? assignee_id
content = I18n.t("conversations.activity.assignee.#{key}", **params)
messages.create(activity_message_params(content))
end
end

73
app/models/conversation.rb
View file

@ -45,6 +45,7 @@ class Conversation < ApplicationRecord
include Labelable
include AssignmentHandler
include RoundRobinHandler
include ActivityMessageHandler
validates :account_id, presence: true
validates :inbox_id, presence: true
@ -72,9 +73,7 @@ class Conversation < ApplicationRecord
before_save :ensure_snooze_until_reset
before_create :mark_conversation_pending_if_bot
# wanted to change this to after_update commit. But it ended up creating a loop
# reinvestigate in future and identity the implications
after_update :notify_status_change, :create_activity
after_update_commit :execute_after_update_commit_callbacks
after_create_commit :notify_conversation_creation, :queue_conversation_auto_resolution_job
after_commit :set_display_id, unless: :display_id?
@ -150,6 +149,11 @@ class Conversation < ApplicationRecord
private
def execute_after_update_commit_callbacks
notify_status_change
create_activity
end
def ensure_snooze_until_reset
self.snoozed_until = nil unless snoozed?
end
@ -168,6 +172,8 @@ class Conversation < ApplicationRecord
end
def queue_conversation_auto_resolution_job
# FIXME: Move this to one cronjob that iterates over accounts and enqueue resolution jobs
# Similar to how we handle campaigns
return unless auto_resolve_duration
AutoResolveConversationsJob.set(wait_until: (last_activity_at || created_at) + auto_resolve_duration.days).perform_later(id)
@ -181,21 +187,6 @@ class Conversation < ApplicationRecord
reload
end
def create_activity
user_name = Current.user.name if Current.user.present?
status_change_activity(user_name) if saved_change_to_status?
create_label_change(user_name) if saved_change_to_label_list?
end
def status_change_activity(user_name)
create_status_change_message(user_name)
queue_conversation_auto_resolution_job if open?
end
def activity_message_params(content)
{ account_id: account_id, inbox_id: inbox_id, message_type: :activity, content: content }
end
def notify_status_change
{
CONVERSATION_OPENED => -> { saved_change_to_status? && open? },
@ -218,16 +209,6 @@ class Conversation < ApplicationRecord
return true if previous_changes.key?(:id) || saved_change_to_status?
end
def create_status_change_message(user_name)
content = if user_name
I18n.t("conversations.activity.status.#{status}", user_name: user_name)
elsif resolved?
I18n.t('conversations.activity.status.auto_resolved', duration: auto_resolve_duration)
end
messages.create(activity_message_params(content)) if content
end
def create_label_change(user_name)
return unless user_name
@ -238,42 +219,6 @@ class Conversation < ApplicationRecord
create_label_removed(user_name, previous_labels - current_labels)
end
def create_label_added(user_name, labels = [])
return unless labels.size.positive?
params = { user_name: user_name, labels: labels.join(', ') }
content = I18n.t('conversations.activity.labels.added', **params)
messages.create(activity_message_params(content))
end
def create_label_removed(user_name, labels = [])
return unless labels.size.positive?
params = { user_name: user_name, labels: labels.join(', ') }
content = I18n.t('conversations.activity.labels.removed', **params)
messages.create(activity_message_params(content))
end
def create_muted_message
return unless Current.user
params = { user_name: Current.user.name }
content = I18n.t('conversations.activity.muted', **params)
messages.create(activity_message_params(content))
end
def create_unmuted_message
return unless Current.user
params = { user_name: Current.user.name }
content = I18n.t('conversations.activity.unmuted', **params)
messages.create(activity_message_params(content))
end
def mute_key
format(Redis::RedisKeys::CONVERSATION_MUTE_KEY, id: id)
end

3
app/models/message.rb
View file

@ -97,7 +97,8 @@ class Message < ApplicationRecord
data = attributes.merge(
created_at: created_at.to_i,
message_type: message_type_before_type_cast,
conversation_id: conversation.display_id
conversation_id: conversation.display_id,
conversation: { assignee_id: conversation.assignee_id }
)
data.merge!(echo_id: echo_id) if echo_id.present?
data.merge!(attachments: attachments.map(&:push_event_data)) if attachments.present?

5
spec/controllers/api/v1/accounts/conversations/assignments_controller_spec.rb
View file

@ -64,7 +64,10 @@ RSpec.describe 'Conversation Assignment API', type: :request do
expect(response).to have_http_status(:success)
expect(conversation.reload.assignee).to eq(nil)
expect(conversation.messages.last.content).to eq("Conversation unassigned by #{agent.name}")
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Conversation unassigned by #{agent.name}" }))
end
end

16
spec/models/concerns/assignment_handler_shared.rb
View file

@ -22,9 +22,13 @@ shared_examples_for 'assignment_handler' do
it 'creates team assigned and unassigned message activity' do
expect(conversation.update(team: team)).to eq true
expect(conversation.messages.pluck(:content)).to include("Assigned to #{team.name} by #{agent.name}")
expect(conversation.update(team: nil)).to eq true
expect(conversation.messages.pluck(:content)).to include("Unassigned from #{team.name} by #{agent.name}")
expect(Conversations::ActivityMessageJob).to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Assigned to #{team.name} by #{agent.name}" }))
expect(Conversations::ActivityMessageJob).to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Unassigned from #{team.name} by #{agent.name}" }))
end
it 'changes assignee to nil if they doesnt belong to the team and allow_auto_assign is false' do
@ -41,7 +45,9 @@ shared_examples_for 'assignment_handler' do
conversation.update(team: team)
expect(conversation.reload.assignee).to eq agent
expect(conversation.messages.pluck(:content)).to include("Assigned to #{conversation.assignee.name} via #{team.name} by #{agent.name}")
expect(Conversations::ActivityMessageJob).to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Assigned to #{conversation.assignee.name} via #{team.name} by #{agent.name}" }))
end
it 'wont change assignee if he is already a team member' do
@ -94,7 +100,9 @@ shared_examples_for 'assignment_handler' do
it 'creates self-assigned message activity' do
expect(update_assignee).to eq(true)
expect(conversation.messages.pluck(:content)).to include("#{agent.name} self-assigned this conversation")
expect(Conversations::ActivityMessageJob).to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id,
message_type: :activity, content: "#{agent.name} self-assigned this conversation" }))
end
end
end

87
spec/models/conversation_spec.rb
View file

@ -72,36 +72,31 @@ RSpec.describe Conversation, type: :model do
end
describe '.after_update' do
let(:account) { create(:account) }
let(:conversation) do
create(:conversation, status: 'open', account: account, assignee: old_assignee)
end
let(:old_assignee) do
let!(:account) { create(:account) }
let!(:old_assignee) do
create(:user, email: 'agent1@example.com', account: account, role: :agent)
end
let(:new_assignee) do
create(:user, email: 'agent2@example.com', account: account, role: :agent)
end
let!(:conversation) do
create(:conversation, status: 'open', account: account, assignee: old_assignee)
end
let(:assignment_mailer) { double(deliver: true) }
let(:label) { create(:label, account: account) }
before do
conversation
new_assignee
allow(Rails.configuration.dispatcher).to receive(:dispatch)
Current.user = old_assignee
end
it 'runs after_update callbacks' do
conversation.update(
status: :resolved,
contact_last_seen_at: Time.now,
assignee: new_assignee,
label_list: [label.title]
)
end
it 'runs after_update callbacks' do
# notify_status_change
expect(Rails.configuration.dispatcher).to have_received(:dispatch)
.with(described_class::CONVERSATION_RESOLVED, kind_of(Time), conversation: conversation)
expect(Rails.configuration.dispatcher).to have_received(:dispatch)
@ -111,19 +106,37 @@ RSpec.describe Conversation, type: :model do
end
it 'creates conversation activities' do
# create_activity
expect(conversation.messages.pluck(:content)).to include("Conversation was marked resolved by #{old_assignee.name}")
expect(conversation.messages.pluck(:content)).to include("Assigned to #{new_assignee.name} by #{old_assignee.name}")
expect(conversation.messages.pluck(:content)).to include("#{old_assignee.name} added #{label.title}")
conversation.update(
status: :resolved,
contact_last_seen_at: Time.now,
assignee: new_assignee,
label_list: [label.title]
)
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "#{old_assignee.name} added #{label.title}" }))
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Conversation was marked resolved by #{old_assignee.name}" }))
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "Assigned to #{new_assignee.name} by #{old_assignee.name}" }))
end
it 'adds a message for system auto resolution if marked resolved by system' do
account.update(auto_resolve_duration: 40)
conversation2 = create(:conversation, status: 'open', account: account, assignee: old_assignee)
Current.user = nil
conversation2.update(status: :resolved)
system_resolved_message = "Conversation was marked resolved by system due to #{account.auto_resolve_duration} days of inactivity"
expect(conversation2.messages.pluck(:content)).to include(system_resolved_message)
expect { conversation2.update(status: :resolved) }
.to have_enqueued_job(Conversations::ActivityMessageJob)
.with(conversation2, { account_id: conversation2.account_id, inbox_id: conversation2.inbox_id, message_type: :activity,
content: system_resolved_message })
end
it 'does not trigger AutoResolutionJob if conversation reopened and account does not have auto resolve duration' do
@ -133,8 +146,9 @@ RSpec.describe Conversation, type: :model do
it 'does trigger AutoResolutionJob if conversation reopened and account has auto resolve duration' do
account.update(auto_resolve_duration: 40)
expect { conversation.reload.update(status: :open) }
.to have_enqueued_job(AutoResolveConversationsJob).with(conversation.id)
conversation.resolved!
conversation.reload.update(status: :open)
expect(AutoResolveConversationsJob).to have_been_enqueued.with(conversation.id)
end
end
@ -161,22 +175,35 @@ RSpec.describe Conversation, type: :model do
it 'adds one label to conversation' do
labels = [first_label].map(&:title)
expect(conversation.update_labels(labels)).to eq(true)
expect { conversation.update_labels(labels) }
.to have_enqueued_job(Conversations::ActivityMessageJob)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "#{agent.name} added #{labels.join(', ')}" })
expect(conversation.label_list).to match_array(labels)
expect(conversation.messages.pluck(:content)).to include("#{agent.name} added #{labels.join(', ')}")
end
it 'adds and removes previously added labels' do
labels = [first_label, fourth_label].map(&:title)
expect(conversation.update_labels(labels)).to eq(true)
expect { conversation.update_labels(labels) }
.to have_enqueued_job(Conversations::ActivityMessageJob)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: "#{agent.name} added #{labels.join(', ')}" })
expect(conversation.label_list).to match_array(labels)
expect(conversation.messages.pluck(:content)).to include("#{agent.name} added #{labels.join(', ')}")
updated_labels = [second_label, third_label].map(&:title)
expect(conversation.update_labels(updated_labels)).to eq(true)
expect(conversation.label_list).to match_array(updated_labels)
expect(conversation.messages.pluck(:content)).to include("#{agent.name} added #{updated_labels.join(', ')}")
expect(conversation.messages.pluck(:content)).to include("#{agent.name} removed #{labels.join(', ')}")
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id,
message_type: :activity, content: "#{agent.name} added #{updated_labels.join(', ')}" }))
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once)
.with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id,
message_type: :activity, content: "#{agent.name} removed #{labels.join(', ')}" }))
end
end
@ -238,7 +265,9 @@ RSpec.describe Conversation, type: :model do
it 'creates mute message' do
mute!
expect(conversation.messages.pluck(:content)).to include("#{user.name} has muted the conversation")
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once).with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id,
message_type: :activity, content: "#{user.name} has muted the conversation" }))
end
end
@ -265,7 +294,9 @@ RSpec.describe Conversation, type: :model do
it 'creates unmute message' do
unmute!
expect(conversation.messages.pluck(:content)).to include("#{user.name} has unmuted the conversation")
expect(Conversations::ActivityMessageJob)
.to(have_been_enqueued.at_least(:once).with(conversation, { account_id: conversation.account_id, inbox_id: conversation.inbox_id,
message_type: :activity, content: "#{user.name} has unmuted the conversation" }))
end
end

12
spec/services/message_templates/hook_execution_service_spec.rb
View file

@ -117,6 +117,9 @@ describe ::MessageTemplates::HookExecutionService do
conversation.inbox.update(csat_survey_enabled: true)
conversation.resolved!
Conversations::ActivityMessageJob.perform_now(conversation,
{ account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: 'Conversation marked resolved!!' })
expect(::MessageTemplates::Template::CsatSurvey).to have_received(:new).with(conversation: conversation)
expect(csat_survey).to have_received(:perform)
@ -126,6 +129,9 @@ describe ::MessageTemplates::HookExecutionService do
conversation.inbox.update(csat_survey_enabled: false)
conversation.resolved!
Conversations::ActivityMessageJob.perform_now(conversation,
{ account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: 'Conversation marked resolved!!' })
expect(::MessageTemplates::Template::CsatSurvey).not_to have_received(:new).with(conversation: conversation)
expect(csat_survey).not_to have_received(:perform)
@ -138,6 +144,9 @@ describe ::MessageTemplates::HookExecutionService do
conversation.inbox.update(csat_survey_enabled: true)
conversation.resolved!
Conversations::ActivityMessageJob.perform_now(conversation,
{ account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: 'Conversation marked resolved!!' })
expect(::MessageTemplates::Template::CsatSurvey).not_to have_received(:new).with(conversation: conversation)
expect(csat_survey).not_to have_received(:perform)
@ -148,6 +157,9 @@ describe ::MessageTemplates::HookExecutionService do
conversation.messages.create!(message_type: 'outgoing', content_type: :input_csat, account: conversation.account, inbox: conversation.inbox)
conversation.resolved!
Conversations::ActivityMessageJob.perform_now(conversation,
{ account_id: conversation.account_id, inbox_id: conversation.inbox_id, message_type: :activity,
content: 'Conversation marked resolved!!' })
expect(::MessageTemplates::Template::CsatSurvey).not_to have_received(:new).with(conversation: conversation)
expect(csat_survey).not_to have_received(:perform)

2
swagger/paths/application/agents/create.yml
View file

@ -28,7 +28,7 @@ parameters:
availability_status:
type: string
enum: ['available', 'busy', 'offline']
description: The availability status of the agent.
description: The availability setting of the agent.
auto_offline:
type: boolean
description: Whether the availability status of agent is configured to go offline automatically when away.

4
swagger/paths/application/agents/update.yml
View file

@ -23,10 +23,10 @@ parameters:
enum: ['agent', 'administrator']
description: Whether its administrator or agent
required: true
availability_status:
availability:
type: string
enum: ['available', 'busy', 'offline']
description: The availability status of the agent.
description: The availability setting of the agent.
auto_offline:
type: boolean
description: Whether the availability status of agent is configured to go offline automatically when away.

6
swagger/swagger.json
View file

@ -1182,7 +1182,7 @@
"busy",
"offline"
],
"description": "The availability status of the agent."
"description": "The availability setting of the agent."
},
"auto_offline": {
"type": "boolean",
@ -1246,14 +1246,14 @@
"description": "Whether its administrator or agent",
"required": true
},
"availability_status": {
"availability": {
"type": "string",
"enum": [
"available",
"busy",
"offline"
],
"description": "The availability status of the agent."
"description": "The availability setting of the agent."
},
"auto_offline": {
"type": "boolean",