Chore: Refactor round robin logic (#1015)

Co-authored-by: Pranav Raj S <pranav@thoughtwoot.com>

.rubocop.yml

@@ -39,6 +39,8 @@ Style/HashTransformKeys:
   Enabled: true
 Style/HashTransformValues:
   Enabled: true
+Style/RedundantFetchBlock:
+  Enabled: true
 Style/RedundantRegexpCharacterClass:
   Enabled: true
 Style/RedundantRegexpEscape:

.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:

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)

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)
+    ::RoundRobin::ManageService.new(inbox: self).clear_queue
-  end
-
-  def round_robin_key
-    format(Constants::RedisKeys::ROUND_ROBIN_AGENTS, inbox_id: id)
   end
 end

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)
+    ::RoundRobin::ManageService.new(inbox: inbox).remove_agent_from_queue(user_id)
-  end
-
-  def round_robin_key
-    format(Constants::RedisKeys::ROUND_ROBIN_AGENTS, inbox_id: inbox_id)
   end
 end

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

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

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

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

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

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.
 #

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

lib/constants/redis_keys.rb

@@ -1,3 +0,0 @@
-module Constants::RedisKeys
-  ROUND_ROBIN_AGENTS = 'ROUND_ROBIN_AGENTS:%{inbox_id}'.freeze
-end

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)

lib/redis/alfred.rb

@@ -1,43 +1,52 @@
 module Redis::Alfred
-  CONVERSATION_MAILER_KEY = 'CONVERSATION::%d'.freeze
+  include Redis::RedisKeys
-
-  # 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
 
   class << self
-    def rpoplpush(source, destination)
+    # key operations
-      $alfred.rpoplpush(source, destination)
-    end
 
-    def lpush(key, value)
+    def set(key, value)
-      $alfred.lpush(key, value)
+      $alfred.set(key, value)
-    end
-
-    def delete(key)
-      $alfred.del(key)
-    end
-
-    def lrem(key, value, count = 0)
-      $alfred.lrem(key, count, 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)

lib/redis/redis_keys.rb

@@ -0,0 +1,13 @@
+module Redis::RedisKeys
+  ROUND_ROBIN_AGENTS = 'ROUND_ROBIN_AGENTS:%<inbox_id>d'.freeze
+
+  CONVERSATION_MAILER_KEY = 'CONVERSATION::%<conversation_id>d'.freeze
+
+  ## Online Status Keys
+  # hash containing user_id key and status as value
+  ONLINE_STATUS = 'ONLINE_STATUS::%<account_id>d'.freeze
+  # sorted set storing online presense of account contacts
+  ONLINE_PRESENCE_CONTACTS = 'ONLINE_PRESENCE::%<account_id>d::CONTACTS'.freeze
+  # sorted set storing online presense of account users
+  ONLINE_PRESENCE_USERS = 'ONLINE_PRESENCE::%<account_id>d::USERS'.freeze
+end

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!

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