diff --git a/app/javascript/dashboard/helper/automationHelper.js b/app/javascript/dashboard/helper/automationHelper.js index 665e8faf6..33e920edc 100644 --- a/app/javascript/dashboard/helper/automationHelper.js +++ b/app/javascript/dashboard/helper/automationHelper.js @@ -121,7 +121,7 @@ export const generateConditionOptions = (options, key = 'id') => { }; // Add the "None" option to the agent list -export const agentList = agents => [ +export const addNoneToList = agents => [ { id: 'nil', name: 'None', @@ -137,8 +137,8 @@ export const getActionOptions = ({ type, }) => { const actionsMap = { - assign_agent: agentList(agents), - assign_team: teams, + assign_agent: addNoneToList(agents), + assign_team: addNoneToList(teams), send_email_to_team: teams, add_label: generateConditionOptions(labels, 'title'), remove_label: generateConditionOptions(labels, 'title'), diff --git a/app/models/concerns/json_schema_validator.rb b/app/models/concerns/json_schema_validator.rb index 4ae94df12..808564be9 100644 --- a/app/models/concerns/json_schema_validator.rb +++ b/app/models/concerns/json_schema_validator.rb @@ -48,7 +48,6 @@ class JsonSchemaValidator < ActiveModel::Validator # Add validation errors to the record with a formatted statement validation_errors.each do |error| - # byebug format_and_append_error(error, record) end end diff --git a/app/services/action_service.rb b/app/services/action_service.rb index 79f1234a0..4f33a302b 100644 --- a/app/services/action_service.rb +++ b/app/services/action_service.rb @@ -50,7 +50,11 @@ class ActionService end def assign_team(team_ids = []) - return unassign_team if team_ids[0]&.zero? + # FIXME: The explicit checks for zero or nil (string) is bad. Move + # this to a separate unassign action. + should_unassign = team_ids.blank? || %w[nil 0].include?(team_ids[0].to_s) + return @conversation.update!(team_id: nil) if should_unassign + # check if team belongs to account only if team_id is present # if team_id is nil, then it means that the team is being unassigned return unless !team_ids[0].nil? && team_belongs_to_account?(team_ids) diff --git a/spec/factories/conversations.rb b/spec/factories/conversations.rb index 6f2bc34ef..c552e6c30 100644 --- a/spec/factories/conversations.rb +++ b/spec/factories/conversations.rb @@ -16,5 +16,17 @@ FactoryBot.define do conversation.contact ||= create(:contact, :with_email, account: conversation.account) conversation.contact_inbox ||= create(:contact_inbox, contact: conversation.contact, inbox: conversation.inbox) end + + trait :with_team do + after(:build) do |conversation| + conversation.team ||= create(:team, account: conversation.account) + end + end + + trait :with_assignee do + after(:build) do |conversation| + conversation.assignee ||= create(:user, account: conversation.account, role: :agent) + end + end end end diff --git a/spec/services/action_service_spec.rb b/spec/services/action_service_spec.rb index bdc9c88ff..c28761844 100644 --- a/spec/services/action_service_spec.rb +++ b/spec/services/action_service_spec.rb @@ -31,18 +31,54 @@ describe ActionService do describe '#assign_agent' do let(:agent) { create(:user, account: account, role: :agent) } - let(:conversation) { create(:conversation, account: account) } let(:inbox_member) { create(:inbox_member, inbox: conversation.inbox, user: agent) } + let(:conversation) { create(:conversation, :with_assignee, account: account) } let(:action_service) { described_class.new(conversation) } it 'unassigns the conversation if agent id is nil' do action_service.assign_agent(['nil']) expect(conversation.reload.assignee).to be_nil end + end - it 'unassigns the team if team_id is nil' do - action_service.assign_team([nil]) - expect(conversation.reload.team).to be_nil + describe '#assign_team' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:inbox_member) { create(:inbox_member, inbox: conversation.inbox, user: agent) } + let(:team) { create(:team, name: 'ConversationTeam', account: account) } + let(:conversation) { create(:conversation, :with_team, account: account) } + let(:action_service) { described_class.new(conversation) } + + context 'when team_id is not present' do + it 'unassign the if team_id is "nil"' do + expect do + action_service.assign_team(['nil']) + end.not_to raise_error + expect(conversation.reload.team).to be_nil + end + + it 'unassign the if team_id is 0' do + expect do + action_service.assign_team([0]) + end.not_to raise_error + expect(conversation.reload.team).to be_nil + end + end + + context 'when team_id is present' do + it 'assign the team if the team is part of the account' do + original_team = conversation.team + expect do + action_service.assign_team([team.id]) + end.to change { conversation.reload.team }.from(original_team) + end + + it 'does not assign the team if the team is part of the account' do + original_team = conversation.team + invalid_team_id = 999_999_999 + expect do + action_service.assign_team([invalid_team_id]) + end.not_to change { conversation.reload.team }.from(original_team) + end end end end