From 823c0ab6a7bdc6ce418787a451ad2423594e2290 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 28 Mar 2022 14:38:07 +0530 Subject: [PATCH] chore: Use Round Robin service for team assignment (#4237) Co-authored-by: Pranav Raj S --- .circleci/config.yml | 2 +- app/models/concerns/assignment_handler.rb | 15 +++---- .../round_robin/assignment_service.rb | 10 +++-- app/services/round_robin/manage_service.rb | 42 +++++++++++++++---- .../assignments_controller_spec.rb | 5 +++ .../round_robin/manage_service_spec.rb | 34 +++++++++++++-- 6 files changed, 86 insertions(+), 22 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5c9b27f03..9b2c68537 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -110,7 +110,7 @@ jobs: - run: name: Run backend tests command: | - bundle exec rspec $(circleci tests glob "spec/**/*_spec.rb" | circleci tests split --split-by=timings) --profile=10 + bundle exec rspec $(circleci tests glob "spec/**/*_spec.rb" | circleci tests split --split-by=timings) --profile=10 --format documentation ~/tmp/cc-test-reporter format-coverage -t simplecov -o ~/tmp/codeclimate.backend.json coverage/backend/.resultset.json - persist_to_workspace: root: ~/tmp diff --git a/app/models/concerns/assignment_handler.rb b/app/models/concerns/assignment_handler.rb index 76ff47aca..72a7438e9 100644 --- a/app/models/concerns/assignment_handler.rb +++ b/app/models/concerns/assignment_handler.rb @@ -12,18 +12,19 @@ module AssignmentHandler def ensure_assignee_is_from_team return unless team_id_changed? - ensure_current_assignee_team - self.assignee_id ||= find_team_assignee_id_for_inbox if team&.allow_auto_assign.present? + validate_current_assignee_team + self.assignee ||= find_assignee_from_team end - def ensure_current_assignee_team + def validate_current_assignee_team self.assignee_id = nil if team&.members&.exclude?(assignee) end - def find_team_assignee_id_for_inbox - members = inbox.members.ids & team.members.ids - # TODO: User round robin to determine the next agent instead of using sample - members.sample + def find_assignee_from_team + return if team&.allow_auto_assign.blank? + + team_members = inbox.members.ids & team.members.ids + ::RoundRobin::AssignmentService.new(conversation: self, allowed_member_ids: team_members).find_assignee end def notify_assignment_change diff --git a/app/services/round_robin/assignment_service.rb b/app/services/round_robin/assignment_service.rb index 26190737f..6cd37b8de 100644 --- a/app/services/round_robin/assignment_service.rb +++ b/app/services/round_robin/assignment_service.rb @@ -1,9 +1,13 @@ class RoundRobin::AssignmentService - pattr_initialize [:conversation] + pattr_initialize [:conversation, { allowed_member_ids: [] }] + + def find_assignee + round_robin_manage_service.available_agent(priority_list: online_agents) + end def perform # online agents will get priority - new_assignee = round_robin_manage_service.available_agent(priority_list: online_agents) + new_assignee = find_assignee conversation.update(assignee: new_assignee) if new_assignee end @@ -15,7 +19,7 @@ class RoundRobin::AssignmentService end def round_robin_manage_service - @round_robin_manage_service ||= RoundRobin::ManageService.new(inbox: conversation.inbox) + @round_robin_manage_service ||= RoundRobin::ManageService.new(inbox: conversation.inbox, allowed_member_ids: allowed_member_ids) end def round_robin_key diff --git a/app/services/round_robin/manage_service.rb b/app/services/round_robin/manage_service.rb index ca12d892a..9b3a60b53 100644 --- a/app/services/round_robin/manage_service.rb +++ b/app/services/round_robin/manage_service.rb @@ -1,5 +1,7 @@ +# If allowed_member_ids are supplied round robin service will only fetch a member from member id +# This is used in case of team assignment class RoundRobin::ManageService - pattr_initialize [:inbox!] + pattr_initialize [:inbox!, { allowed_member_ids: [] }] # called on inbox delete def clear_queue @@ -18,9 +20,9 @@ class RoundRobin::ManageService def available_agent(priority_list: []) reset_queue unless validate_queue? - user_id = get_agent_via_priority_list(priority_list) + user_id = get_member_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) + user_id ||= fetch_user_id inbox.inbox_members.find_by(user_id: user_id)&.user if user_id.present? end @@ -31,17 +33,36 @@ class RoundRobin::ManageService private - def get_agent_via_priority_list(priority_list) + def fetch_user_id + if allowed_member_ids_in_str.present? + user_id = queue.intersection(allowed_member_ids_in_str).pop + pop_push_to_queue(user_id) + user_id + else + ::Redis::Alfred.rpoplpush(round_robin_key, round_robin_key) + end + end + + # priority list is usually the members who are online passed from assignmebt service + def get_member_via_priority_list(priority_list) + return if priority_list.blank? + + # when allowed member ids is passed we will be looking to get members from that list alone + priority_list = priority_list.intersection(allowed_member_ids_in_str) if allowed_member_ids_in_str.present? 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 + pop_push_to_queue(user_id) user_id end + def pop_push_to_queue(user_id) + return if user_id.blank? + + remove_agent_from_queue(user_id) + add_agent_to_queue(user_id) + end + def validate_queue? return true if inbox.inbox_members.map(&:user_id).sort == queue.map(&:to_i).sort end @@ -53,4 +74,9 @@ class RoundRobin::ManageService def round_robin_key format(::Redis::Alfred::ROUND_ROBIN_AGENTS, inbox_id: inbox.id) end + + def allowed_member_ids_in_str + # NOTE: the values which are returned from redis for priority list are string + @allowed_member_ids_in_str ||= allowed_member_ids.map(&:to_s) + end end diff --git a/spec/controllers/api/v1/accounts/conversations/assignments_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations/assignments_controller_spec.rb index 5d65c70ca..6583eaf0e 100644 --- a/spec/controllers/api/v1/accounts/conversations/assignments_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations/assignments_controller_spec.rb @@ -35,6 +35,9 @@ RSpec.describe 'Conversation Assignment API', type: :request do end it 'assigns a team to the conversation' do + team_member = create(:user, account: account, role: :agent) + create(:inbox_member, inbox: conversation.inbox, user: team_member) + create(:team_member, team: team, user: team_member) params = { team_id: team.id } post api_v1_account_conversation_assignments_url(account_id: account.id, conversation_id: conversation.display_id), @@ -44,6 +47,8 @@ RSpec.describe 'Conversation Assignment API', type: :request do expect(response).to have_http_status(:success) expect(conversation.reload.team).to eq(team) + # assignee will be from team + expect(conversation.reload.assignee).to eq(team_member) end end diff --git a/spec/services/round_robin/manage_service_spec.rb b/spec/services/round_robin/manage_service_spec.rb index 0deef3109..75110d325 100644 --- a/spec/services/round_robin/manage_service_spec.rb +++ b/spec/services/round_robin/manage_service_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe RoundRobin::ManageService do - subject(:round_robin_manage_service) { ::RoundRobin::ManageService.new(inbox: inbox) } + subject(:round_robin_manage_service) { described_class.new(inbox: inbox) } let!(:account) { create(:account) } let!(:inbox) { create(:inbox, account: account) } @@ -18,8 +18,9 @@ describe RoundRobin::ManageService do 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(round_robin_manage_service.available_agent(priority_list: [inbox_members[3].user_id, - inbox_members[2].user_id])).to eq inbox_members[2].user + # prority list will be ids in string, since thats what redis supplies to us + expect(round_robin_manage_service.available_agent(priority_list: [inbox_members[3].user_id.to_s, + inbox_members[2].user_id.to_s])).to eq inbox_members[2].user expect(round_robin_manage_service.send(:queue)).to eq(expected_queue) end @@ -39,5 +40,32 @@ describe RoundRobin::ManageService do # the service have refreshed the redis queue before performing expect(round_robin_manage_service.send(:queue).sort.map(&:to_i)).to eq(inbox_members.map(&:user_id).sort) end + + context 'when allowed_member_ids is passed' do + it 'will get the first allowed member and move it to the end of the queue' do + expected_queue = [inbox_members[3].user_id, inbox_members[2].user_id, inbox_members[4].user_id, inbox_members[1].user_id, + inbox_members[0].user_id].map(&:to_s) + expect(described_class.new(inbox: inbox, + allowed_member_ids: [inbox_members[3].user_id, + inbox_members[2].user_id]).available_agent).to eq inbox_members[2].user + expect(described_class.new(inbox: inbox, + allowed_member_ids: [inbox_members[3].user_id, + inbox_members[2].user_id]).available_agent).to eq inbox_members[3].user + expect(round_robin_manage_service.send(:queue)).to eq(expected_queue) + end + + it 'will get union of priority list and allowed_member_ids and move it to the end of the queue' do + expected_queue = [inbox_members[3].user_id, inbox_members[4].user_id, inbox_members[2].user_id, inbox_members[1].user_id, + inbox_members[0].user_id].map(&:to_s) + # prority list will be ids in string, since thats what redis supplies to us + expect(described_class.new(inbox: inbox, + allowed_member_ids: [inbox_members[3].user_id, + inbox_members[2].user_id]) + .available_agent( + priority_list: [inbox_members[3].user_id.to_s] + )).to eq inbox_members[3].user + expect(round_robin_manage_service.send(:queue)).to eq(expected_queue) + end + end end end