Refactor Conversation model (#134)

* Add Conversation factory with dependent factories

* Include FactoryBot methods in rspec config

* Add unit tests for public methods of Conversation model

* Move Current model into a separate file in lib folder

* Disable Metrics/BlockLength rule for db/migrate and spec folders

* Get rid of global $dispatcher variable

* Create Message#unread_since scope

* Refactor callback methods in Conversation model

* Create Conversations::EventDataPresenter

* Add translation keys for activity messages

* Add pry-rails gem

* Refactor Conversation#notify_status_change

* Add mock_redis for test env
This commit is contained in:
Emil Shakirov 2019-10-12 20:08:41 +02:00 committed by Sojan Jose
parent 43e54a7bfb
commit 4768aca484
25 changed files with 490 additions and 99 deletions

View file

@ -6,3 +6,7 @@ Style/FrozenStringLiteralComment:
Enabled: false
Style/SymbolArray:
Enabled: false
Metrics/BlockLength:
Exclude:
- db/migrate/**/*
- spec/**/*

View file

@ -56,10 +56,16 @@ group :development do
gem 'web-console'
end
group :test do
gem 'mock_redis'
end
group :development, :test do
gem 'byebug', platform: :mri
gem 'factory_bot_rails'
gem 'listen'
gem 'pry-rails'
gem 'rspec-rails', '~> 3.8'
gem 'rubocop', '~> 0.73.0', require: false
gem 'seed_dump'

View file

@ -165,6 +165,7 @@ GEM
json_pure (~> 2.1)
rest-client (>= 1.8, < 3.0)
cliver (0.3.2)
coderay (1.1.2)
coercible (1.0.0)
descendants_tracker (~> 0.0.1)
coffee-rails (5.0.0)
@ -266,6 +267,7 @@ GEM
mini_mime (1.0.2)
mini_portile2 (2.4.0)
minitest (5.12.2)
mock_redis (0.22.0)
msgpack (1.3.1)
multi_json (1.13.1)
multi_xml (0.6.0)
@ -298,6 +300,11 @@ GEM
capybara (>= 2.1, < 4)
cliver (~> 0.3.1)
websocket-driver (>= 0.2.0)
pry (0.12.2)
coderay (~> 1.1.0)
method_source (~> 0.9.0)
pry-rails (0.3.9)
pry (>= 0.10.4)
public_suffix (4.0.1)
puma (3.12.1)
pundit (2.1.0)
@ -477,10 +484,12 @@ DEPENDENCIES
letter_opener
listen
mini_magick
mock_redis
nightfury (~> 1.0, >= 1.0.1)
omniauth-facebook
pg
poltergeist
pry-rails
puma (~> 3.0)
pundit
pusher

View file

@ -1,7 +1,3 @@
module Current
thread_mattr_accessor :user
end
class ApplicationController < ActionController::Base
include DeviseTokenAuth::Concerns::SetUserByToken
include Pundit

View file

@ -4,7 +4,7 @@ class Dispatcher
attr_reader :async_dispatcher, :sync_dispatcher
def self.dispatch(event_name, timestamp, data, async = false)
$dispatcher.dispatch(event_name, timestamp, data, async)
Rails.configuration.dispatcher.dispatch(event_name, timestamp, data, async)
end
def initialize

View file

@ -53,10 +53,10 @@ class Account < ApplicationRecord
end
def notify_creation
$dispatcher.dispatch(ACCOUNT_CREATED, Time.zone.now, account: self)
Rails.configuration.dispatcher.dispatch(ACCOUNT_CREATED, Time.zone.now, account: self)
end
def notify_deletion
$dispatcher.dispatch(ACCOUNT_DESTROYED, Time.zone.now, account: self)
Rails.configuration.dispatcher.dispatch(ACCOUNT_DESTROYED, Time.zone.now, account: self)
end
end

View file

@ -19,89 +19,73 @@ class Conversation < ApplicationRecord
before_create :set_display_id, unless: :display_id?
after_update :notify_status_change,
:create_activity,
:send_email_notification_to_assignee
after_update :notify_status_change, :create_activity, :send_email_notification_to_assignee
after_create :send_events, :run_round_robin
acts_as_taggable_on :labels
def update_assignee(agent = nil)
self.assignee = agent
save!
update!(assignee: agent)
end
def update_labels(labels = nil)
self.label_list = labels
save!
update!(label_list: labels)
end
def toggle_status
self.status = open? ? :resolved : :open
save! ? true : false
save
end
def lock!
self.locked = true
save!
update!(locked: true)
end
def unlock!
self.locked = false
save!
update!(locked: false)
end
def unread_messages
# +1 is a hack to avoid https://makandracards.com/makandra/1057-why-two-ruby-time-objects-are-not-equal-although-they-appear-to-be
# ente budhiparamaya neekam kandit entu tonunu?
messages.where('EXTRACT(EPOCH FROM created_at) > (?)', agent_last_seen_at.to_i + 1)
messages.unread_since(agent_last_seen_at)
end
def unread_incoming_messages
messages.incoming.where('EXTRACT(EPOCH FROM created_at) > (?)', agent_last_seen_at.to_i + 1)
messages.incoming.unread_since(agent_last_seen_at)
end
def push_event_data
{
meta: { sender: sender.push_event_data, assignee: assignee }, id: display_id,
messages: [messages.chat.last&.push_event_data], inbox_id: inbox_id, status: status_before_type_cast.to_i,
timestamp: created_at.to_i, user_last_seen_at: user_last_seen_at.to_i, agent_last_seen_at: agent_last_seen_at.to_i,
unread_count: unread_incoming_messages.count
}
Conversations::EventDataPresenter.new(self).push_data
end
def lock_event_data
{
id: display_id,
locked: locked?
}
Conversations::EventDataPresenter.new(self).lock_data
end
private
def dispatch_events
$dispatcher.dispatch(CONVERSATION_RESOLVED, Time.zone.now, conversation: self)
dispatcher_dispatch(CONVERSATION_RESOLVED)
end
def send_events
$dispatcher.dispatch(CONVERSATION_CREATED, Time.zone.now, conversation: self)
dispatcher_dispatch(CONVERSATION_CREATED)
end
def send_email_notification_to_assignee
AssignmentMailer.conversation_assigned(self, assignee).deliver if assignee_id_changed? && assignee_id.present? && !self_assign?(assignee_id)
return if self_assign?(assignee_id)
AssignmentMailer.conversation_assigned(self, assignee).deliver if saved_change_to_assignee_id? && assignee_id.present?
end
def self_assign?(assignee_id)
return false unless Current.user
Current.user.id == assignee_id
assignee_id.present? && Current.user&.id == assignee_id
end
def set_display_id
self.display_id = loop do
disp_id = account.conversations.maximum('display_id').to_i + 1
break disp_id unless account.conversations.exists?(display_id: disp_id)
next_display_id = account.conversations.maximum('display_id').to_i + 1
break next_display_id unless account.conversations.exists?(display_id: next_display_id)
end
end
@ -110,78 +94,48 @@ class Conversation < ApplicationRecord
user_name = Current.user&.name
create_status_change_message(user_name) if status_changed?
create_assignee_change(username) if assignee_id_changed?
end
def status_changed_message
return "Conversation was marked resolved by #{Current.user.try(:name)}" if resolved?
"Conversation was reopened by #{Current.user.try(:name)}"
end
def assignee_changed_message
return "Assigned to #{assignee.name} by #{Current.user.try(:name)}" if assignee_id
"Conversation unassigned by #{Current.user.try(:name)}"
create_status_change_message(user_name) if saved_change_to_assignee_id?
create_assignee_change(user_name) if saved_change_to_assignee_id?
end
def activity_message_params(content)
{
account_id: account_id,
inbox_id: inbox_id,
message_type: :activity,
content: content
}
{ account_id: account_id, inbox_id: inbox_id, message_type: :activity, content: content }
end
def notify_status_change
resolve_conversation if status_changed?
dispatcher_dispatch(CONVERSATION_READ) if user_last_seen_at_changed?
dispatcher_dispatch(CONVERSATION_LOCK_TOGGLE) if locked_changed?
dispatcher_dispatch(ASSIGNEE_CHANGED) if assignee_id_changed?
end
def resolve_conversation
if resolved? && assignee.present?
dispatcher_dispatch(CONVERSATION_RESOLVED)
{
CONVERSATION_RESOLVED => -> { saved_change_to_status? && resolved? && assignee.present? },
CONVERSATION_READ => -> { saved_change_to_user_last_seen_at? },
CONVERSATION_LOCK_TOGGLE => -> { saved_change_to_locked? },
ASSIGNEE_CHANGED => -> { saved_change_to_assignee_id? }
}.each do |event, condition|
condition.call && dispatcher_dispatch(event)
end
end
def dispatcher_dispatch(event_name)
$dispatcher.dispatch(event_name, Time.zone.now, conversation: self)
Rails.configuration.dispatcher.dispatch(event_name, Time.zone.now, conversation: self)
end
def run_round_robin
return unless true # conversation.account.has_feature?(round_robin)
return unless true # conversation.account.round_robin_enabled?
# return unless conversation.account.has_feature?(round_robin)
# return unless conversation.account.round_robin_enabled?
return if assignee
new_assignee = inbox.next_available_agent
update_assignee(new_assignee) if new_assignee
inbox.next_available_agent.then { |new_assignee| update_assignee(new_assignee) }
end
def create_status_change_message(user_name)
content = if resolved?
"Conversation was marked resolved by #{user_name}"
else
"Conversation was reopened by #{user_name}"
end
content = I18n.t("conversations.activity.status.#{status}", user_name: user_name)
messages.create(activity_message_params(content))
end
def create_assignee_change(username)
content = if assignee_id
"Assigned to #{assignee.name} by #{username}"
else
"Conversation unassigned by #{username}"
end
def create_assignee_change(user_name)
params = { assignee_name: assignee&.name, user_name: user_name }.compact
key = assignee_id ? 'assigned' : 'removed'
content = I18n.t("conversations.activity.assignee.#{key}", params)
messages.create(activity_message_params(content))
end
def resolved_and_assignee?
resolved? && assignee.present?
end
end

View file

@ -8,6 +8,8 @@ class Message < ApplicationRecord
enum message_type: [ :incoming, :outgoing, :activity ]
enum status: [ :sent, :delivered, :read, :failed ]
# .succ is a hack to avoid https://makandracards.com/makandra/1057-why-two-ruby-time-objects-are-not-equal-although-they-appear-to-be
scope :unread_since, ->(datetime) { where('EXTRACT(EPOCH FROM created_at) > (?)', datetime.to_i.succ) }
scope :chat, -> { where.not(message_type: :activity, private: true) }
default_scope { order(created_at: :asc) }
@ -42,10 +44,10 @@ class Message < ApplicationRecord
private
def dispatch_event
$dispatcher.dispatch(MESSAGE_CREATED, Time.zone.now, message: self) unless self.conversation.messages.count == 1
Rails.configuration.dispatcher.dispatch(MESSAGE_CREATED, Time.zone.now, message: self) unless self.conversation.messages.count == 1
if outgoing? && self.conversation.messages.outgoing.count == 1
$dispatcher.dispatch(FIRST_REPLY_CREATED, Time.zone.now, message: self)
Rails.configuration.dispatcher.dispatch(FIRST_REPLY_CREATED, Time.zone.now, message: self)
end
end
@ -56,7 +58,7 @@ class Message < ApplicationRecord
def reopen_conversation
if incoming? && self.conversation.resolved?
self.conversation.toggle_status
$dispatcher.dispatch(CONVERSATION_REOPENED, Time.zone.now, conversation: self.conversation)
Rails.configuration.dispatcher.dispatch(CONVERSATION_REOPENED, Time.zone.now, conversation: self.conversation)
end
end
end

View file

@ -36,6 +36,6 @@ class Subscription < ApplicationRecord
end
def notify_creation
$dispatcher.dispatch(SUBSCRIPTION_CREATED, Time.zone.now, subscription: self)
Rails.configuration.dispatcher.dispatch(SUBSCRIPTION_CREATED, Time.zone.now, subscription: self)
end
end

View file

@ -43,11 +43,11 @@ class User < ApplicationRecord
end
def notify_creation
$dispatcher.dispatch(AGENT_ADDED, Time.zone.now, account: self.account)
Rails.configuration.dispatcher.dispatch(AGENT_ADDED, Time.zone.now, account: self.account)
end
def notify_deletion
$dispatcher.dispatch(AGENT_REMOVED, Time.zone.now, account: self.account)
Rails.configuration.dispatcher.dispatch(AGENT_REMOVED, Time.zone.now, account: self.account)
end
def push_event_data

View file

@ -0,0 +1,37 @@
module Conversations
class EventDataPresenter < SimpleDelegator
def lock_data
{ id: display_id, locked: locked? }
end
def push_data
{
id: display_id,
inbox_id: inbox_id,
messages: push_messages,
meta: push_meta,
status: status_before_type_cast.to_i,
unread_count: unread_incoming_messages.count,
**push_timestamps
}
end
private
def push_messages
[messages.chat.last&.push_event_data].compact
end
def push_meta
{ sender: sender.push_event_data, assignee: assignee }
end
def push_timestamps
{
agent_last_seen_at: agent_last_seen_at.to_i,
user_last_seen_at: user_last_seen_at.to_i,
timestamp: created_at.to_i
}
end
end
end

View file

@ -1,2 +1,6 @@
$dispatcher = Dispatcher.instance
$dispatcher.load_listeners
Rails.application.configure do
config.to_prepare do
Rails.configuration.dispatcher = Dispatcher.instance
Rails.configuration.dispatcher.load_listeners
end
end

View file

@ -1,5 +1,5 @@
uri = URI.parse(ENV['REDIS_URL'])
redis = Redis.new(:url => uri)
redis = Rails.env.test? ? MockRedis.new : Redis.new(:url => uri)
Nightfury.redis = Redis::Namespace.new("reports", redis: redis)
=begin

View file

@ -41,3 +41,12 @@ en:
invalid_email: You have entered an invalid email
email_already_exists: "You have already signed up for an account with %{email}"
failed: Signup failed
conversations:
activity:
status:
resolved: "Conversation was marked resolved by %{user_name}"
open: "Conversation was reopened by #{user_name}"
assignee:
assigned: "Assigned to %{assignee_name} by %{user_name}"
removed: "Conversation unassigned by %{user_name}"

3
lib/current.rb Normal file
View file

@ -0,0 +1,3 @@
module Current
thread_mattr_accessor :user
end

View file

@ -0,0 +1,7 @@
# frozen_string_literal: true
FactoryBot.define do
factory :account do
sequence(:name) { |n| "Account #{n}" }
end
end

View file

@ -0,0 +1,9 @@
# frozen_string_literal: true
FactoryBot.define do
factory :channel_widget, class: 'Channel::Widget' do
sequence(:website_name) { |n| "Example Website #{n}" }
sequence(:website_url) { |n| "https://example-#{n}.com" }
account
end
end

View file

@ -0,0 +1,16 @@
# frozen_string_literal: true
FactoryBot.define do
factory :contact do
sequence(:name) { |n| "Widget #{n}" }
sequence(:email) { |n| "widget-#{n}@example.com" }
phone_number { "+123456789011" }
source_id { rand(100) }
chat_channel { "chat_channel" }
account
inbox
end
end

View file

@ -0,0 +1,24 @@
# frozen_string_literal: true
FactoryBot.define do
factory :conversation do
status { 'open' }
display_id { SecureRandom.uuid }
user_last_seen_at { Time.current }
agent_last_seen_at { Time.current }
locked { false }
factory :complete_conversation do
after(:build) do |conversation|
conversation.account ||= create(:account)
conversation.inbox ||= create(
:inbox,
account: conversation.account,
channel: create(:channel_widget, account: conversation.account)
)
conversation.sender ||= create(:contact, account: conversation.account)
conversation.assignee ||= create(:user)
end
end
end
end

View file

@ -0,0 +1,9 @@
# frozen_string_literal: true
FactoryBot.define do
factory :inbox do
account
association :channel, factory: :channel_widget
name { "Inbox" }
end
end

View file

@ -0,0 +1,14 @@
# frozen_string_literal: true
FactoryBot.define do
factory :message do
content { 'Message' }
status { 'sent' }
message_type { 'incoming' }
fb_id { SecureRandom.uuid }
account
inbox
conversation
user
end
end

14
spec/factories/users.rb Normal file
View file

@ -0,0 +1,14 @@
# frozen_string_literal: true
FactoryBot.define do
factory :user do
provider { 'email' }
uid { SecureRandom.uuid }
name { 'John Smith' }
nickname { 'jsmith' }
email { 'john.smith@example.com' }
role { 'agent' }
password { "password" }
account
end
end

View file

@ -0,0 +1,238 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe Conversation, type: :model do
describe '.before_create' do
let(:conversation) { build(:complete_conversation, display_id: nil) }
before do
conversation.save
conversation.reload
end
it 'runs before_create callbacks' do
expect(conversation.display_id).to eq(1)
end
end
describe '.after_update' do
let(:account) { create(:account) }
let(:conversation) do
create(:complete_conversation, status: 'open', account: account, assignee: old_assignee)
end
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(:assignment_mailer) { double(deliver: true) }
before do
conversation
new_assignee
allow(Rails.configuration.dispatcher).to receive(:dispatch)
allow(AssignmentMailer).to receive(:conversation_assigned).and_return(assignment_mailer)
allow(assignment_mailer).to receive(:deliver)
Current.user = old_assignee
conversation.update(
status: :resolved,
locked: true,
user_last_seen_at: Time.now,
assignee: new_assignee
)
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)
.with(described_class::CONVERSATION_READ, kind_of(Time), conversation: conversation)
expect(Rails.configuration.dispatcher).to have_received(:dispatch)
.with(described_class::CONVERSATION_LOCK_TOGGLE, kind_of(Time), conversation: conversation)
expect(Rails.configuration.dispatcher).to have_received(:dispatch)
.with(described_class::ASSIGNEE_CHANGED, kind_of(Time), conversation: conversation)
# create_activity
expect(conversation.messages.pluck(:content)).to eq(
[
'Conversation was marked resolved by John Smith',
'Assigned to John Smith by John Smith'
]
)
# send_email_notification_to_assignee
expect(AssignmentMailer).to have_received(:conversation_assigned).with(conversation, new_assignee)
expect(assignment_mailer).to have_received(:deliver)
end
end
describe '.after_create' do
let(:account) { create(:account) }
let(:agent) { create(:user, email: 'agent1@example.com', account: account) }
let(:inbox) { create(:inbox, account: account) }
let(:conversation) do
create(
:conversation,
account: account,
sender: create(:contact, account: account),
inbox: inbox,
assignee: nil
)
end
before do
allow(Rails.configuration.dispatcher).to receive(:dispatch)
allow(Redis::Alfred).to receive(:rpoplpush).and_return(agent.id)
end
it 'runs after_create callbacks' do
# send_events
expect(Rails.configuration.dispatcher).to have_received(:dispatch)
.with(described_class::CONVERSATION_CREATED, kind_of(Time), conversation: conversation)
# run_round_robin
expect(conversation.reload.assignee).to eq(agent)
end
end
describe '#update_assignee' do
subject(:update_assignee) { conversation.update_assignee(agent) }
let(:conversation) { create(:complete_conversation, assignee: nil) }
let(:agent) do
create(:user, email: 'agent@example.com', account: conversation.account, role: :agent)
end
it 'assigns the agent to conversation' do
expect(update_assignee).to eq(true)
expect(conversation.reload.assignee).to eq(agent)
end
end
describe '#toggle_status' do
subject(:toggle_status) { conversation.toggle_status }
let(:conversation) { create(:complete_conversation, status: :open) }
it 'toggles conversation status' do
expect(toggle_status).to eq(true)
expect(conversation.reload.status).to eq('resolved')
end
end
describe '#lock!' do
subject(:lock!) { conversation.lock! }
let(:conversation) { create(:complete_conversation) }
it 'assigns locks the conversation' do
expect(lock!).to eq(true)
expect(conversation.reload.locked).to eq(true)
end
end
describe '#unlock!' do
subject(:unlock!) { conversation.unlock! }
let(:conversation) { create(:complete_conversation) }
it 'unlocks the conversation' do
expect(unlock!).to eq(true)
expect(conversation.reload.locked).to eq(false)
end
end
describe 'unread_messages' do
subject(:unread_messages) { conversation.unread_messages }
let(:conversation) { create(:complete_conversation, agent_last_seen_at: 1.hour.ago) }
let(:message_params) do
{
conversation: conversation,
account: conversation.account,
inbox: conversation.inbox,
user: conversation.assignee
}
end
let!(:message) do
create(:message, created_at: 1.minute.ago, **message_params)
end
before do
create(:message, created_at: 1.month.ago, **message_params)
end
it 'returns unread messages' do
expect(unread_messages).to contain_exactly(message)
end
end
describe 'unread_incoming_messages' do
subject(:unread_incoming_messages) { conversation.unread_incoming_messages }
let(:conversation) { create(:complete_conversation, agent_last_seen_at: 1.hour.ago) }
let(:message_params) do
{
conversation: conversation,
account: conversation.account,
inbox: conversation.inbox,
user: conversation.assignee,
created_at: 1.minute.ago
}
end
let!(:message) do
create(:message, message_type: :incoming, **message_params)
end
before do
create(:message, message_type: :outgoing, **message_params)
end
it 'returns unread incoming messages' do
expect(unread_incoming_messages).to contain_exactly(message)
end
end
describe '#push_event_data' do
subject(:push_event_data) { conversation.push_event_data }
let(:conversation) { create(:complete_conversation) }
let(:expected_data) do
{
meta: {
sender: conversation.sender.push_event_data,
assignee: conversation.assignee
},
id: conversation.display_id,
messages: [],
inbox_id: conversation.inbox_id,
status: conversation.status_before_type_cast.to_i,
timestamp: conversation.created_at.to_i,
user_last_seen_at: conversation.user_last_seen_at.to_i,
agent_last_seen_at: conversation.agent_last_seen_at.to_i,
unread_count: 0
}
end
it 'returns push event payload' do
expect(push_event_data).to eq(expected_data)
end
end
describe '#lock_event_data' do
subject(:lock_event_data) { conversation.lock_event_data }
let(:conversation) do
build(:conversation, display_id: 505, locked: false)
end
it 'returns lock event payload' do
expect(lock_event_data).to eq(id: 505, locked: false)
end
end
end

View file

@ -0,0 +1,35 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe Conversations::EventDataPresenter do
let(:presenter) { described_class.new(conversation) }
let(:conversation) { create(:complete_conversation) }
describe '#lock_data' do
it { expect(presenter.lock_data).to eq(id: conversation.display_id, locked: false) }
end
describe '#push_data' do
let(:expected_data) do
{
meta: {
sender: conversation.sender.push_event_data,
assignee: conversation.assignee
},
id: conversation.display_id,
messages: [],
inbox_id: conversation.inbox_id,
status: conversation.status_before_type_cast.to_i,
timestamp: conversation.created_at.to_i,
user_last_seen_at: conversation.user_last_seen_at.to_i,
agent_last_seen_at: conversation.agent_last_seen_at.to_i,
unread_count: 0
}
end
it 'returns push event payload' do
expect(presenter.push_data).to eq(expected_data)
end
end
end

View file

@ -30,6 +30,7 @@ rescue ActiveRecord::PendingMigrationError => e
exit 1
end
RSpec.configure do |config|
config.include FactoryBot::Syntax::Methods
# Remove this line if you're not using ActiveRecord or ActiveRecord fixtures
config.fixture_path = "#{::Rails.root}/spec/fixtures"