From 254c8c1e4bbe23f1e79e7e1586e61a6713ddc553 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 12 Sep 2018 06:23:48 +0200 Subject: [PATCH] Improved error handling of system_agent_limit setting it the number was supplied as string. --- app/models/role.rb | 8 +- app/models/user.rb | 8 +- spec/models/user_spec.rb | 125 ++++++++++++++++++++ test/unit/role_validate_agent_limit_test.rb | 69 +++++++++++ 4 files changed, 202 insertions(+), 8 deletions(-) diff --git a/app/models/role.rb b/app/models/role.rb index 5e60cfd31..48b18bcba 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -186,7 +186,7 @@ returns end def validate_agent_limit_by_attributes - return true if !Setting.get('system_agent_limit') + return true if Setting.get('system_agent_limit').blank? return true if !will_save_change_to_attribute?('active') return true if active != true return true if !with_permission?('ticket.agent') @@ -194,19 +194,19 @@ returns currents = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).distinct().pluck(:id) news = User.joins(:roles).where(roles: { id: id }, users: { active: true }).distinct().pluck(:id) count = currents.concat(news).uniq.count - raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit') + raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit').to_i true end def validate_agent_limit_by_permission(permission) - return true if !Setting.get('system_agent_limit') + return true if Setting.get('system_agent_limit').blank? return true if active != true return true if permission.active != true return true if permission.name != 'ticket.agent' ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent' }, roles: { active: true }).pluck(:id) ticket_agent_role_ids.push(id) count = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).distinct().count - raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit') + raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit').to_i true end diff --git a/app/models/user.rb b/app/models/user.rb index 515243d77..4e78f2106 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1066,18 +1066,18 @@ raise 'Minimum one user need to have admin permissions' end def validate_agent_limit_by_attributes - return true if !Setting.get('system_agent_limit') + return true if Setting.get('system_agent_limit').blank? return true if !will_save_change_to_attribute?('active') return true if active != true return true if !permissions?('ticket.agent') ticket_agent_role_ids = Role.joins(:permissions).where(permissions: { name: 'ticket.agent', active: true }, roles: { active: true }).pluck(:id) count = User.joins(:roles).where(roles: { id: ticket_agent_role_ids }, users: { active: true }).distinct().count + 1 - raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit') + raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit').to_i true end def validate_agent_limit_by_role(role) - return true if !Setting.get('system_agent_limit') + return true if Setting.get('system_agent_limit').blank? return true if active != true return true if role.active != true return true if !role.with_permission?('ticket.agent') @@ -1100,7 +1100,7 @@ raise 'Minimum one user need to have admin permissions' count += 1 end end - raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit') + raise Exceptions::UnprocessableEntity, 'Agent limit exceeded, please check your account settings.' if count > Setting.get('system_agent_limit').to_i true end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index dfca9cfcc..6d2df30fc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -665,6 +665,131 @@ RSpec.describe User do end end end + + context '#validate_agent_limit_by_role by string' do + + context 'agent creation limit not reached' do + + it 'grants agent creation' do + Setting.set('system_agent_limit', (current_agent_count + 1).to_s) + + expect do + create(:agent_user) + end.to change { + current_agent_count + }.by(1) + end + + it 'grants role change' do + Setting.set('system_agent_limit', (current_agent_count + 1).to_s) + + future_agent = create(:customer_user) + + expect do + future_agent.roles = [agent_role] + end.to change { + current_agent_count + }.by(1) + end + + context 'role updates' do + + it 'grants update by instances' do + Setting.set('system_agent_limit', (current_agent_count + 1).to_s) + + agent = create(:agent_user) + + expect do + agent.roles = [ + admin_role, + agent_role + ] + agent.save! + end.not_to raise_error + end + + it 'grants update by id (Integer)' do + Setting.set('system_agent_limit', (current_agent_count + 1).to_s) + + agent = create(:agent_user) + + expect do + agent.role_ids = [ + admin_role.id, + agent_role.id + ] + agent.save! + end.not_to raise_error + end + + it 'grants update by id (String)' do + Setting.set('system_agent_limit', (current_agent_count + 1).to_s) + + agent = create(:agent_user) + + expect do + agent.role_ids = [ + admin_role.id.to_s, + agent_role.id.to_s + ] + agent.save! + end.not_to raise_error + end + end + end + + context 'agent creation limit surpassing prevention' do + + it 'creation of new agents' do + Setting.set('system_agent_limit', (current_agent_count + 2).to_s) + + create_list(:agent_user, 2) + + initial_agent_count = current_agent_count + + expect do + create(:agent_user) + end.to raise_error(Exceptions::UnprocessableEntity) + + expect(current_agent_count).to eq(initial_agent_count) + end + + it 'prevents role change' do + Setting.set('system_agent_limit', current_agent_count.to_s) + + future_agent = create(:customer_user) + + initial_agent_count = current_agent_count + + expect do + future_agent.roles = [agent_role] + end.to raise_error(Exceptions::UnprocessableEntity) + + expect(current_agent_count).to eq(initial_agent_count) + end + end + end + + context '#validate_agent_limit_by_attributes' do + + context 'agent creation limit surpassing prevention' do + + it 'prevents re-activation of agents' do + Setting.set('system_agent_limit', current_agent_count.to_s) + + inactive_agent = create(:agent_user, active: false) + + initial_agent_count = current_agent_count + + expect do + inactive_agent.update!(active: true) + end.to raise_error(Exceptions::UnprocessableEntity) + + expect(current_agent_count).to eq(initial_agent_count) + end + end + end + end context 'when phone attribute' do diff --git a/test/unit/role_validate_agent_limit_test.rb b/test/unit/role_validate_agent_limit_test.rb index 4292e596b..11bdceab6 100644 --- a/test/unit/role_validate_agent_limit_test.rb +++ b/test/unit/role_validate_agent_limit_test.rb @@ -71,6 +71,75 @@ class RoleValidateAgentLimit < ActiveSupport::TestCase Setting.set('system_agent_limit', nil) end + test 'role validate agent limit as string' do + + agent_max = User.with_permissions('ticket.agent').count + UserInfo.current_user_id = 1 + Setting.set('system_agent_limit', (agent_max + 2).to_s) + + permission_ticket_agent = Permission.where(name: 'ticket.agent') + + role_agent_limit_success = Role.create!( + name: 'agent-limit-test-success', + note: 'agent-limit-test-success Role.', + permissions: [], + active: true, + ) + role_agent_limit_fail = Role.create!( + name: 'agent-limit-test-fail', + note: 'agent-limit-test-fail Role.', + permissions: [], + active: true, + ) + + user1 = User.create!( + firstname: 'Firstname', + lastname: 'Lastname', + email: 'some-agentlimit-role@example.com', + login: 'some-agentlimit-role@example.com', + roles: [role_agent_limit_success], + active: true, + ) + user2 = User.create!( + firstname: 'Firstname1', + lastname: 'Lastname1', + email: 'some-agentlimit-role-1@example.com', + login: 'some-agentlimit-role-1@example.com', + roles: [role_agent_limit_success], + active: true, + ) + user3 = User.create!( + firstname: 'Firstname2', + lastname: 'Lastname2', + email: 'some-agentlimit-role-2@example.com', + login: 'some-agentlimit-role-2@example.com', + roles: [role_agent_limit_fail], + active: true, + ) + + role_agent_limit_success.permissions = permission_ticket_agent + assert_raises(Exceptions::UnprocessableEntity) do + role_agent_limit_fail.permissions = permission_ticket_agent + end + + role_agent_limit_fail.active = false + role_agent_limit_fail.save! + + role_agent_limit_fail.permissions = permission_ticket_agent + + assert_raises(Exceptions::UnprocessableEntity) do + role_agent_limit_fail.active = true + role_agent_limit_fail.save! + end + + user1.destroy! + user2.destroy! + user3.destroy! + role_agent_limit_success.destroy! + role_agent_limit_fail.destroy! + Setting.set('system_agent_limit', nil) + end + test 'role validate agent limit - 1 user 2 ticket.agent roles' do current_agent_max = User.with_permissions('ticket.agent').count + 1