From 0fc0dc1683a7090ead71f298aeef4635a173d776 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 8 Jul 2020 00:14:07 +0530 Subject: [PATCH] Chore: Refactor round robin logic (#1015) Co-authored-by: Pranav Raj S --- .rubocop.yml | 2 + .rubocop_todo.yml | 18 ------ app/models/conversation.rb | 2 +- app/models/inbox.rb | 11 +--- app/models/inbox_member.rb | 8 +-- app/models/message.rb | 5 +- .../round_robin/assignment_service.rb | 24 +++++++ app/services/round_robin/manage_service.rb | 56 +++++++++++++++++ .../conversation_reply_email_worker.rb | 7 ++- config/initializers/redis.rb | 3 +- config/puma.rb | 4 +- db/migrate/20170525104650_round_robin.rb | 2 +- lib/constants/redis_keys.rb | 3 - lib/online_status_tracker.rb | 6 +- lib/redis/alfred.rb | 63 +++++++++++-------- lib/redis/redis_keys.rb | 13 ++++ spec/models/conversation_spec.rb | 3 + .../round_robin/manage_service_spec.rb | 41 ++++++++++++ 18 files changed, 197 insertions(+), 74 deletions(-) create mode 100644 app/services/round_robin/assignment_service.rb create mode 100644 app/services/round_robin/manage_service.rb delete mode 100644 lib/constants/redis_keys.rb create mode 100644 lib/redis/redis_keys.rb create mode 100644 spec/services/round_robin/manage_service_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 4d75629e8..ab5efd6c8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -39,6 +39,8 @@ Style/HashTransformKeys: Enabled: true Style/HashTransformValues: Enabled: true +Style/RedundantFetchBlock: + Enabled: true Style/RedundantRegexpCharacterClass: Enabled: true Style/RedundantRegexpEscape: diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 2569a6d66..2d3abda7f 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -266,24 +266,6 @@ Style/CommentedKeyword: - 'app/controllers/api/v1/conversations/labels_controller.rb' - 'app/controllers/api/v1/labels_controller.rb' -# Offense count: 1 -# Configuration parameters: EnforcedStyle. -# SupportedStyles: annotated, template, unannotated -Style/FormatStringToken: - Exclude: - - 'lib/constants/redis_keys.rb' - -# Offense count: 4 -# Configuration parameters: AllowedVariables. -Style/GlobalVars: - Exclude: - - 'lib/redis/alfred.rb' - -# Offense count: 4 -Style/IdenticalConditionalBranches: - Exclude: - - 'app/controllers/api/v1/reports_controller.rb' - # Offense count: 1 # Configuration parameters: AllowIfModifier. Style/IfInsideElse: diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 4798d36b3..8da783235 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -188,7 +188,7 @@ class Conversation < ApplicationRecord return unless conversation_status_changed_to_open? return unless should_round_robin? - inbox.next_available_agent.then { |new_assignee| update_assignee(new_assignee) } + ::RoundRobin::AssignmentService.new(conversation: self).perform end def create_status_change_message(user_name) diff --git a/app/models/inbox.rb b/app/models/inbox.rb index b48b6b354..060dee980 100644 --- a/app/models/inbox.rb +++ b/app/models/inbox.rb @@ -64,11 +64,6 @@ class Inbox < ApplicationRecord channel.class.name.to_s == 'Channel::WebWidget' end - def next_available_agent - user_id = Redis::Alfred.rpoplpush(round_robin_key, round_robin_key) - account.users.find_by(id: user_id) - end - def webhook_data { id: id, @@ -79,10 +74,6 @@ class Inbox < ApplicationRecord private def delete_round_robin_agents - Redis::Alfred.delete(round_robin_key) - end - - def round_robin_key - format(Constants::RedisKeys::ROUND_ROBIN_AGENTS, inbox_id: id) + ::RoundRobin::ManageService.new(inbox: self).clear_queue end end diff --git a/app/models/inbox_member.rb b/app/models/inbox_member.rb index 4c1e14159..4326b0a8f 100644 --- a/app/models/inbox_member.rb +++ b/app/models/inbox_member.rb @@ -26,14 +26,10 @@ class InboxMember < ApplicationRecord private def add_agent_to_round_robin - Redis::Alfred.lpush(round_robin_key, user_id) + ::RoundRobin::ManageService.new(inbox: inbox).add_agent_to_queue(user_id) end def remove_agent_from_round_robin - Redis::Alfred.lrem(round_robin_key, user_id) - end - - def round_robin_key - format(Constants::RedisKeys::ROUND_ROBIN_AGENTS, inbox_id: inbox_id) + ::RoundRobin::ManageService.new(inbox: inbox).remove_agent_from_queue(user_id) end end diff --git a/app/models/message.rb b/app/models/message.rb index 0ec98ac2e..7b368fe93 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -153,7 +153,6 @@ class Message < ApplicationRecord end def notify_via_mail - conversation_mail_key = Redis::Alfred::CONVERSATION_MAILER_KEY % conversation.id if Redis::Alfred.get(conversation_mail_key).nil? && conversation.contact.email? && outgoing? # set a redis key for the conversation so that we don't need to send email for every # new message that comes in and we dont enque the delayed sidekiq job for every message @@ -165,6 +164,10 @@ class Message < ApplicationRecord end end + def conversation_mail_key + format(::Redis::Alfred::CONVERSATION_MAILER_KEY, conversation_id: conversation.id) + end + def validate_attachments_limit(_attachment) errors.add(attachments: 'exceeded maximum allowed') if attachments.size >= NUMBER_OF_PERMITTED_ATTACHMENTS end diff --git a/app/services/round_robin/assignment_service.rb b/app/services/round_robin/assignment_service.rb new file mode 100644 index 000000000..26190737f --- /dev/null +++ b/app/services/round_robin/assignment_service.rb @@ -0,0 +1,24 @@ +class RoundRobin::AssignmentService + pattr_initialize [:conversation] + + def perform + # online agents will get priority + new_assignee = round_robin_manage_service.available_agent(priority_list: online_agents) + conversation.update(assignee: new_assignee) if new_assignee + end + + private + + def online_agents + online_agents = OnlineStatusTracker.get_available_users(conversation.account_id) + online_agents.select { |_key, value| value.eql?('online') }.keys if online_agents.present? + end + + def round_robin_manage_service + @round_robin_manage_service ||= RoundRobin::ManageService.new(inbox: conversation.inbox) + end + + def round_robin_key + format(::Redis::Alfred::ROUND_ROBIN_AGENTS, inbox_id: conversation.inbox_id) + end +end diff --git a/app/services/round_robin/manage_service.rb b/app/services/round_robin/manage_service.rb new file mode 100644 index 000000000..3720b1d6d --- /dev/null +++ b/app/services/round_robin/manage_service.rb @@ -0,0 +1,56 @@ +class RoundRobin::ManageService + pattr_initialize [:inbox!] + + # called on inbox delete + def clear_queue + ::Redis::Alfred.delete(round_robin_key) + end + + # called on inbox member create + def add_agent_to_queue(user_id) + ::Redis::Alfred.lpush(round_robin_key, user_id) + end + + # called on inbox member delete + def remove_agent_from_queue(user_id) + ::Redis::Alfred.lrem(round_robin_key, user_id) + end + + def available_agent(priority_list: []) + reset_queue unless validate_queue? + user_id = get_agent_via_priority_list(priority_list) + # incase priority list was empty or inbox members weren't present + user_id ||= ::Redis::Alfred.rpoplpush(round_robin_key, round_robin_key) + inbox.inbox_members.find_by(user_id: user_id)&.user if user_id.present? + end + + def reset_queue + clear_queue + add_agent_to_queue(inbox.inbox_members.map(&:user_id)) + end + + private + + def get_agent_via_priority_list(priority_list) + return if priority_list.blank? + + user_id = queue.intersection(priority_list.map(&:to_s)).pop + if user_id.present? + remove_agent_from_queue(user_id) + add_agent_to_queue(user_id) + end + user_id + end + + def validate_queue? + return true if inbox.inbox_members.map(&:user_id).sort == queue.sort.map(&:to_i) + end + + def queue + ::Redis::Alfred.lrange(round_robin_key) + end + + def round_robin_key + format(::Redis::Alfred::ROUND_ROBIN_AGENTS, inbox_id: inbox.id) + end +end diff --git a/app/workers/conversation_reply_email_worker.rb b/app/workers/conversation_reply_email_worker.rb index 8fed7cc12..e63d8fafb 100644 --- a/app/workers/conversation_reply_email_worker.rb +++ b/app/workers/conversation_reply_email_worker.rb @@ -9,7 +9,12 @@ class ConversationReplyEmailWorker ConversationReplyMailer.reply_with_summary(@conversation, queued_time).deliver_later # delete the redis set from the first new message on the conversation - conversation_mail_key = Redis::Alfred::CONVERSATION_MAILER_KEY % @conversation.id Redis::Alfred.delete(conversation_mail_key) end + + private + + def conversation_mail_key + format(::Redis::Alfred::CONVERSATION_MAILER_KEY, conversation_id: @conversation.id) + end end diff --git a/config/initializers/redis.rb b/config/initializers/redis.rb index c059323aa..423e0500f 100644 --- a/config/initializers/redis.rb +++ b/config/initializers/redis.rb @@ -4,8 +4,9 @@ app_redis_config = { } redis = Rails.env.test? ? MockRedis.new : Redis.new(app_redis_config) -# Alfred - Used currently for round robin and conversation emails. +# Alfred # Add here as you use it for more features +# Used for Round Robin, Conversation Emails & Online Presence $alfred = Redis::Namespace.new('alfred', redis: redis, warning: true) # https://github.com/mperham/sidekiq/issues/4591 diff --git a/config/puma.rb b/config/puma.rb index 0359054fb..1df88f6db 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -4,13 +4,13 @@ # the maximum value specified for Puma. Default is set to 5 threads for minimum # and maximum; this matches the default thread size of Active Record. # -max_threads_count = ENV.fetch('RAILS_MAX_THREADS') { 5 } +max_threads_count = ENV.fetch('RAILS_MAX_THREADS', 5) min_threads_count = ENV.fetch('RAILS_MIN_THREADS') { max_threads_count } threads min_threads_count, max_threads_count # Specifies the `port` that Puma will listen on to receive requests; default is 3000. # -port ENV.fetch('PORT') { 3000 } +port ENV.fetch('PORT', 3000) # Specifies the `environment` that Puma will run in. # diff --git a/db/migrate/20170525104650_round_robin.rb b/db/migrate/20170525104650_round_robin.rb index 3938346c9..e0476438a 100644 --- a/db/migrate/20170525104650_round_robin.rb +++ b/db/migrate/20170525104650_round_robin.rb @@ -1,7 +1,7 @@ class RoundRobin < ActiveRecord::Migration[5.0] def change InboxMember.find_each do |im| - round_robin_key = format(Constants::RedisKeys::ROUND_ROBIN_AGENTS, inbox_id: im.inbox_id) + round_robin_key = format(::Redis::Alfred::ROUND_ROBIN_AGENTS, inbox_id: im.inbox_id) Redis::Alfred.lpush(round_robin_key, im.user_id) end end diff --git a/lib/constants/redis_keys.rb b/lib/constants/redis_keys.rb deleted file mode 100644 index 7138e4f24..000000000 --- a/lib/constants/redis_keys.rb +++ /dev/null @@ -1,3 +0,0 @@ -module Constants::RedisKeys - ROUND_ROBIN_AGENTS = 'ROUND_ROBIN_AGENTS:%{inbox_id}'.freeze -end diff --git a/lib/online_status_tracker.rb b/lib/online_status_tracker.rb index c9180e96f..622585dff 100644 --- a/lib/online_status_tracker.rb +++ b/lib/online_status_tracker.rb @@ -16,9 +16,9 @@ module OnlineStatusTracker def self.presence_key(account_id, type) case type when 'Contact' - Redis::Alfred::ONLINE_PRESENCE_CONTACTS % account_id + format(::Redis::Alfred::ONLINE_PRESENCE_CONTACTS, account_id: account_id) else - Redis::Alfred::ONLINE_PRESENCE_USERS % account_id + format(::Redis::Alfred::ONLINE_PRESENCE_USERS, account_id: account_id) end end @@ -34,7 +34,7 @@ module OnlineStatusTracker end def self.status_key(account_id) - Redis::Alfred::ONLINE_STATUS % account_id + format(::Redis::Alfred::ONLINE_STATUS, account_id: account_id) end def self.get_available_contacts(account_id) diff --git a/lib/redis/alfred.rb b/lib/redis/alfred.rb index e1274a59b..d023f9fa3 100644 --- a/lib/redis/alfred.rb +++ b/lib/redis/alfred.rb @@ -1,43 +1,52 @@ module Redis::Alfred - CONVERSATION_MAILER_KEY = 'CONVERSATION::%d'.freeze - - # hash containing user_id key and status as value ONLINE_STATUS::%accountid - ONLINE_STATUS = 'ONLINE_STATUS::%s'.freeze - # sorted set storing online presense of account contacts : ONLINE_PRESENCE::%accountid::CONTACTS - ONLINE_PRESENCE_CONTACTS = 'ONLINE_PRESENCE::%s::CONTACTS'.freeze - # sorted set storing online presense of account users : ONLINE_PRESENCE::%accountid::USERS - ONLINE_PRESENCE_USERS = 'ONLINE_PRESENCE::%s::USERS'.freeze + include Redis::RedisKeys class << self - def rpoplpush(source, destination) - $alfred.rpoplpush(source, destination) - end + # key operations - def lpush(key, value) - $alfred.lpush(key, value) - end - - def delete(key) - $alfred.del(key) - end - - def lrem(key, value, count = 0) - $alfred.lrem(key, count, value) + def set(key, value) + $alfred.set(key, value) end def setex(key, value, expiry = 1.day) $alfred.setex(key, expiry, value) end - def set(key, value) - $alfred.set(key, value) - end - def get(key) $alfred.get(key) end - # hash operation + def delete(key) + $alfred.del(key) + end + + # list operations + + def llen(key) + $alfred.llen(key) + end + + def lrange(key, start_index = 0, end_index = -1) + $alfred.lrange(key, start_index, end_index) + end + + def rpop(key) + $alfred.rpop(key) + end + + def lpush(key, values) + $alfred.lpush(key, values) + end + + def rpoplpush(source, destination) + $alfred.rpoplpush(source, destination) + end + + def lrem(key, value, count = 0) + $alfred.lrem(key, count, value) + end + + # hash operations # add a key value to redis hash def hset(key, field, value) @@ -54,7 +63,7 @@ module Redis::Alfred $alfred.hmget(key, *fields) end - # sorted set functions + # sorted set operations # add score and value for a key def zadd(key, score, value) diff --git a/lib/redis/redis_keys.rb b/lib/redis/redis_keys.rb new file mode 100644 index 000000000..417978d7c --- /dev/null +++ b/lib/redis/redis_keys.rb @@ -0,0 +1,13 @@ +module Redis::RedisKeys + ROUND_ROBIN_AGENTS = 'ROUND_ROBIN_AGENTS:%d'.freeze + + CONVERSATION_MAILER_KEY = 'CONVERSATION::%d'.freeze + + ## Online Status Keys + # hash containing user_id key and status as value + ONLINE_STATUS = 'ONLINE_STATUS::%d'.freeze + # sorted set storing online presense of account contacts + ONLINE_PRESENCE_CONTACTS = 'ONLINE_PRESENCE::%d::CONTACTS'.freeze + # sorted set storing online presense of account users + ONLINE_PRESENCE_USERS = 'ONLINE_PRESENCE::%d::USERS'.freeze +end diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb index 6e2e79be1..1c5cc2939 100644 --- a/spec/models/conversation_spec.rb +++ b/spec/models/conversation_spec.rb @@ -108,6 +108,7 @@ RSpec.describe Conversation, type: :model do end before do + create(:inbox_member, inbox: inbox, user: agent) allow(Redis::Alfred).to receive(:rpoplpush).and_return(agent.id) end @@ -141,9 +142,11 @@ RSpec.describe Conversation, type: :model do conversation.status = 'resolved' conversation.save! expect(conversation.reload.assignee).to eq(agent) + inbox.inbox_members.where(user_id: agent.id).first.destroy! # round robin changes assignee in this case since agent doesn't have access to inbox agent2 = create(:user, email: 'agent2@example.com', account: account) + create(:inbox_member, inbox: inbox, user: agent2) allow(Redis::Alfred).to receive(:rpoplpush).and_return(agent2.id) conversation.status = 'open' conversation.save! diff --git a/spec/services/round_robin/manage_service_spec.rb b/spec/services/round_robin/manage_service_spec.rb new file mode 100644 index 000000000..8d0696b26 --- /dev/null +++ b/spec/services/round_robin/manage_service_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +describe RoundRobin::ManageService do + let!(:account) { create(:account) } + let!(:inbox) { create(:inbox, account: account) } + let!(:inbox_members) { create_list(:inbox_member, 5, inbox: inbox) } + let(:subject) { ::RoundRobin::ManageService.new(inbox: inbox) } + + describe '#available_agent' do + it 'gets the first available agent and move agent to end of the list' do + expected_queue = [inbox_members[0].user_id, inbox_members[4].user_id, inbox_members[3].user_id, inbox_members[2].user_id, + inbox_members[1].user_id].map(&:to_s) + subject.available_agent + expect(subject.send(:queue)).to eq(expected_queue) + end + + it 'gets intersection of priority list and agent queue. get and move agent to the end of the list' do + expected_queue = [inbox_members[2].user_id, inbox_members[4].user_id, inbox_members[3].user_id, inbox_members[1].user_id, + inbox_members[0].user_id].map(&:to_s) + expect(subject.available_agent(priority_list: [inbox_members[3].user_id, inbox_members[2].user_id])).to eq inbox_members[2].user + expect(subject.send(:queue)).to eq(expected_queue) + end + + it 'constructs round_robin_queue if queue is not present' do + subject.clear_queue + expect(subject.send(:queue)).to eq([]) + subject.available_agent + # the service constructed the redis queue before performing + expect(subject.send(:queue).sort.map(&:to_i)).to eq(inbox_members.map(&:user_id).sort) + end + + it 'validates the queue and correct it before performing round robin' do + # adding some invalid ids to queue + subject.add_agent_to_queue([2, 3, 5, 9]) + expect(subject.send(:queue).sort.map(&:to_i)).not_to eq(inbox_members.map(&:user_id).sort) + subject.available_agent + # the service have refreshed the redis queue before performing + expect(subject.send(:queue).sort.map(&:to_i)).to eq(inbox_members.map(&:user_id).sort) + end + end +end