Refactoring: Migrate role_test.rb to RSpec.

app/models/permission.rb

@@ -31,4 +31,8 @@ returns
     names
   end
 
+  def to_s
+    name
+  end
+
 end

app/models/role.rb

@@ -9,12 +9,16 @@ class Role < ApplicationModel
   include Role::Assets
 
   has_and_belongs_to_many :users, after_add: :cache_update, after_remove: :cache_update
-  has_and_belongs_to_many :permissions, after_add: :cache_update, after_remove: :cache_update, before_update: :cache_update, after_update: :cache_update, before_add: :validate_agent_limit_by_permission, before_remove: :last_admin_check_by_permission
+  has_and_belongs_to_many :permissions,
+                          before_add:    %i[validate_agent_limit_by_permission validate_permissions],
+                          after_add:     :cache_update,
+                          before_remove: :last_admin_check_by_permission,
+                          after_remove:  :cache_update
   validates               :name,  presence: true
   store                   :preferences
 
-  before_create  :validate_permissions, :check_default_at_signup_permissions
+  before_create  :check_default_at_signup_permissions
-  before_update  :validate_permissions, :last_admin_check_by_attribute, :validate_agent_limit_by_attributes, :check_default_at_signup_permissions
+  before_update  :last_admin_check_by_attribute, :validate_agent_limit_by_attributes, :check_default_at_signup_permissions
 
   # ignore Users because this will lead to huge
   # results for e.g. the Customer role
@@ -150,23 +154,17 @@ returns
 
   private
 
-  def validate_permissions
+  def validate_permissions(permission)
-    Rails.logger.debug { "self permission: #{self.permission_ids}" }
+    Rails.logger.debug { "self permission: #{permission.id}" }
-    return true if !self.permission_ids
 
-    permission_ids.each do |permission_id|
+    raise "Permission #{permission.name} is disabled" if permission.preferences[:disabled]
-      permission = Permission.lookup(id: permission_id)
-      raise "Unable to find permission for id #{permission_id}" if !permission
-      raise "Permission #{permission.name} is disabled" if permission.preferences[:disabled] == true
-      next if !permission.preferences[:not]
 
-      permission.preferences[:not].each do |local_permission_name|
+    permission.preferences[:not]
-        local_permission = Permission.lookup(name: local_permission_name)
+              &.find { |name| name.in?(permissions.map(&:name)) }
-        next if !local_permission
+              &.tap { |conflict| raise "Permission #{permission} conflicts with #{conflict}" }
-        raise "Permission #{permission.name} conflicts with #{local_permission.name}" if permission_ids.include?(local_permission.id)
+
-      end
+    permissions.find { |p| p.preferences[:not]&.include?(permission.name) }
-    end
+               &.tap { |conflict| raise "Permission #{permission} conflicts with #{conflict}" }
-    true
   end
 
   def last_admin_check_by_attribute

spec/factories/permission.rb

@@ -0,0 +1,5 @@
+FactoryBot.define do
+  factory :permission do
+    name { Faker::Job.unique.position.downcase }
+  end
+end

spec/factories/role.rb

@@ -5,10 +5,13 @@ FactoryBot.define do
 end
 
 FactoryBot.define do
-
   factory :role do
     name { generate(:test_role_name) }
     created_by_id 1
     updated_by_id 1
+
+    factory :agent_role do
+      permissions { Permission.where(name: 'ticket.agent') }
+    end
   end
 end

spec/models/role_spec.rb

@@ -3,31 +3,247 @@ require 'models/concerns/has_groups_examples'
 
 RSpec.describe Role do
   include_examples 'HasGroups', group_access_factory: :role
+  subject(:role) { create(:role) }
 
-  context '#validate_agent_limit_by_attributes' do
+  describe 'Default state' do
+    describe 'of whole table:' do
+      it 'has three records ("Admin", "Agent", and "Customer")' do
+        expect(Role.pluck(:name)).to match_array(%w[Admin Agent Customer])
+      end
+    end
 
-    context 'agent creation limit surpassing prevention' do
+    describe 'of "Admin" role:' do
+      it 'has default admin permissions' do
+        expect(Role.find_by(name: 'Admin').permissions.pluck(:name))
+          .to match_array(%w[admin user_preferences report])
+      end
+    end
 
-      def current_agent_count
+    describe 'of "Agent" role:' do
-        User.with_permissions('ticket.agent').count
+      it 'has default agent permissions' do
+        expect(Role.find_by(name: 'Agent').permissions.pluck(:name))
+          .to match_array(%w[ticket.agent chat.agent cti.agent user_preferences])
+      end
+    end
+
+    describe 'of "Customer" role:' do
+      it 'has default customer permissions' do
+        expect(Role.find_by(name: 'Customer').permissions.pluck(:name))
+          .to match_array(
+            %w[
+              user_preferences.password
+              user_preferences.language
+              user_preferences.linked_accounts
+              user_preferences.avatar
+              ticket.customer
+            ]
+          )
+      end
+    end
+  end
+
+  describe 'Callbacks -' do
+    describe 'Permission validation:' do
+      context 'with normal permission' do
+        let(:permission) { create(:permission) }
+
+        it 'can be created' do
+          expect { create(:role, permissions: [permission]) }
+            .to change { Role.count }.by(1)
+        end
+
+        it 'can be added' do
+          expect { role.permissions << permission }
+            .to change { role.permissions.count }.by(1)
+        end
       end
 
-      it 'prevents re-activation of Role with agent permission' do
+      context 'with disabled permission' do
-        Setting.set('system_agent_limit', current_agent_count)
+        let(:permission) { create(:permission, preferences: { disabled: true }) }
 
-        inactive_agent_role = create(:role,
+        it 'cannot be created' do
-                                     active:      false,
+          expect { create(:role, permissions: [permission]) }
-                                     permissions: Permission.where(name: 'ticket.agent'))
+            .to raise_error(/is disabled/)
+            .and change { Role.count }.by(0)
+        end
 
-        create(:user, roles: [inactive_agent_role])
+        it 'cannot be added' do
+          expect { role.permissions << permission }
+            .to raise_error(/is disabled/)
+            .and change { role.permissions.count }.by(0)
+        end
+      end
 
-        initial_agent_count = current_agent_count
+      context 'with multiple, explicitly incompatible permissions' do
+        let(:permission) { create(:permission, preferences: { not: [Permission.first.name] }) }
 
-        expect do
+        it 'cannot be created' do
-          inactive_agent_role.update!(active: true)
+          expect { create(:role, permissions: [Permission.first, permission]) }
-        end.to raise_error(Exceptions::UnprocessableEntity)
+            .to raise_error(/conflicts with/)
+            .and change { Role.count }.by(0)
+        end
 
-        expect(current_agent_count).to eq(initial_agent_count)
+        it 'cannot be added' do
+          role.permissions << Permission.first
+
+          expect { role.permissions << permission }
+            .to raise_error(/conflicts with/)
+            .and change { role.permissions.count }.by(0)
+        end
+      end
+
+      context 'with multiple, compatible permissions' do
+        let(:permission) { create(:permission, preferences: { not: [Permission.pluck(:name).max.next] }) }
+
+        it 'can be created' do
+          expect { create(:role, permissions: [Permission.first, permission]) }
+            .to change { Role.count }.by(1)
+        end
+
+        it 'can be added' do
+          role.permissions << Permission.first
+
+          expect { role.permissions << permission }
+            .to change { role.permissions.count }.by(1)
+        end
+      end
+    end
+
+    describe 'System-wide agent limit checks:' do
+      let(:agents) { User.with_permissions('ticket.agent') }
+
+      describe '#validate_agent_limit_by_attributes' do
+        context 'when reactivating a role adds new agents' do
+          before { create(:user, roles: [role])  }
+          subject(:role) { create(:agent_role, active: false) }
+
+          context 'exceeding the system limit' do
+            before { Setting.set('system_agent_limit', agents.count) }
+
+            it 'fails and raises an error' do
+              expect { role.update!(active: true) }
+                .to raise_error(Exceptions::UnprocessableEntity)
+                .and change { agents.count }.by(0)
+            end
+          end
+        end
+      end
+    end
+
+    describe 'Restrictions on #default_at_signup:' do
+      context 'for roles with "admin" permissions' do
+        subject(:role) { build(:role, permissions: Permission.where(name: 'admin')) }
+
+        it 'cannot be set to true on creation' do
+          role.default_at_signup = true
+
+          expect { role.save }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+
+        it 'cannot be changed to true' do
+          role.save
+
+          expect { role.update(default_at_signup: true) }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+      end
+
+      context 'for roles with permissions that are children of "admin"' do
+        subject(:role) { build(:role, permissions: [permission]) }
+        let(:permission) { create(:permission, name: 'admin.foo') }
+
+        it 'cannot be set to true on creation' do
+          role.default_at_signup = true
+
+          expect { role.save }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+
+        it 'cannot be changed to true' do
+          role.save
+
+          expect { role.update(default_at_signup: true) }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+      end
+
+      context 'for roles with "ticket.agent" permissions' do
+        subject(:role) { build(:role, permissions: Permission.where(name: 'ticket.agent')) }
+
+        it 'cannot be set to true on creation' do
+          role.default_at_signup = true
+
+          expect { role.save }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+
+        it 'cannot be changed to true' do
+          role.save
+
+          expect { role.update(default_at_signup: true) }
+            .to raise_error(Exceptions::UnprocessableEntity, /Cannot set default at signup/)
+        end
+      end
+    end
+  end
+
+  describe '.with_permissions' do
+    context 'when given a name not matching any permissions' do
+      let(:permission) { 'foo' }
+      let(:result) { [] }
+
+      it 'returns an empty array' do
+        expect(Role.with_permissions(permission)).to match_array(result)
+      end
+    end
+
+    context 'when given the name of a top-level permission' do
+      let(:permission) { 'user_preferences' }
+      let(:result) { Role.where(name: %w[Admin Agent]) }
+
+      it 'returns an array of roles with that permission' do
+        expect(Role.with_permissions(permission)).to match_array(result)
+      end
+    end
+
+    context 'when given the name of a child permission' do
+      let(:permission) { 'user_preferences.language' }
+      let(:result) { Role.all }
+
+      it 'returns an array of roles with either that permission or an ancestor' do
+        expect(Role.with_permissions(permission)).to match_array(result)
+      end
+    end
+
+    context 'when given the names of multiple permissions' do
+      let(:permissions) { %w[ticket.agent ticket.customer] }
+      let(:result) { Role.where(name: %w[Agent Customer]) }
+
+      it 'returns an array of roles matching ANY given permission' do
+        expect(Role.with_permissions(permissions)).to match_array(result)
+      end
+    end
+  end
+
+  describe '#with_permission?' do
+    subject(:role) { Role.find_by(name: 'Admin') }
+
+    context 'when given the name of a permission it has' do
+      it 'returns true' do
+        expect(role.with_permission?('admin')).to be(true)
+      end
+    end
+
+    context 'when given the name of a permission it does NOT have' do
+      it 'returns false' do
+        expect(role.with_permission?('ticket.customer')).to be(false)
+      end
+    end
+
+    context 'when given the name of multiple permissions' do
+      it 'returns true as long as ANY match' do
+        expect(role.with_permission?(['admin', 'ticket.customer'])).to be(true)
       end
     end
   end

test/unit/role_test.rb

@@ -1,223 +0,0 @@
-require 'test_helper'
-
-class RoleTest < ActiveSupport::TestCase
-  test 'permission' do
-
-    permission_test = Permission.create_or_update(
-      name:        'test',
-      note:        'parent test permission',
-      preferences: {
-        disabled: true
-      },
-    )
-    permission_test_agent = Permission.create_or_update(
-      name:        'test.agent',
-      note:        'agent test permission',
-      preferences: {
-        not: ['test.customer'],
-      },
-    )
-    permission_test_customer = Permission.create_or_update(
-      name:        'test.customer',
-      note:        'customer test permission',
-      preferences: {
-        not: ['test.agent'],
-      },
-    )
-    permission_test_normal = Permission.create_or_update(
-      name:        'test.normal',
-      note:        'normal test permission',
-      preferences: {},
-    )
-
-    assert_raises(RuntimeError) do
-      Role.create(
-        name:          'Test1',
-        note:          'Test1 Role.',
-        permissions:   [permission_test],
-        updated_by_id: 1,
-        created_by_id: 1
-      )
-    end
-    assert_raises(RuntimeError) do
-      Role.create(
-        name:          'Test1',
-        note:          'Test1 Role.',
-        permissions:   [permission_test_agent, permission_test_customer],
-        updated_by_id: 1,
-        created_by_id: 1
-      )
-    end
-    assert_raises(RuntimeError) do
-      Role.create(
-        name:          'Test1',
-        note:          'Test1 Role.',
-        permissions:   [permission_test_normal, permission_test_agent, permission_test_customer],
-        updated_by_id: 1,
-        created_by_id: 1
-      )
-    end
-    role11 = Role.create(
-      name:          'Test1.1',
-      note:          'Test1.1 Role.',
-      permissions:   [permission_test_agent],
-      updated_by_id: 1,
-      created_by_id: 1
-    )
-    role12 = Role.create(
-      name:          'Test1.2',
-      note:          'Test1.2 Role.',
-      permissions:   [permission_test_customer],
-      updated_by_id: 1,
-      created_by_id: 1
-    )
-    role13 = Role.create(
-      name:          'Test1.3',
-      note:          'Test1.3 Role.',
-      permissions:   [permission_test_normal],
-      updated_by_id: 1,
-      created_by_id: 1
-    )
-    role14 = Role.create(
-      name:          'Test1.4',
-      note:          'Test1.4 Role.',
-      permissions:   [permission_test_normal, permission_test_customer],
-      updated_by_id: 1,
-      created_by_id: 1
-    )
-
-  end
-
-  test 'permission default' do
-    roles = Role.with_permissions('not_existing')
-    assert(roles.blank?)
-
-    roles = Role.with_permissions('admin')
-    assert_equal('Admin', roles.first.name)
-
-    roles = Role.with_permissions('admin.session')
-    assert_equal('Admin', roles.first.name)
-
-    roles = Role.with_permissions(['admin.session', 'not_existing'])
-    assert_equal('Admin', roles.first.name)
-
-    roles = Role.with_permissions('ticket.agent')
-    assert_equal('Agent', roles.first.name)
-
-    roles = Role.with_permissions(['ticket.agent', 'not_existing'])
-    assert_equal('Agent', roles.first.name)
-
-    roles = Role.with_permissions('ticket.customer')
-    assert_equal('Customer', roles.first.name)
-
-    roles = Role.with_permissions(['ticket.customer', 'not_existing'])
-    assert_equal('Customer', roles.first.name)
-
-  end
-
-  test 'with permission' do
-    permission_test1 = Permission.create_or_update(
-      name: 'test-with-permission1',
-      note: 'parent test permission 1',
-    )
-    permission_test2 = Permission.create_or_update(
-      name: 'test-with-permission2',
-      note: 'parent test permission 2',
-    )
-    name = rand(999_999_999)
-    role = Role.create(
-      name:          "Test with Permission? #{name}",
-      note:          "Test with Permission? #{name} Role.",
-      permissions:   [permission_test2],
-      updated_by_id: 1,
-      created_by_id: 1
-    )
-    assert_not(role.with_permission?('test-with-permission1'))
-    assert(role.with_permission?('test-with-permission2'))
-    assert(role.with_permission?(['test-with-permission2', 'some_other_permission']))
-  end
-
-  test 'default_at_signup' do
-
-    agent_role = Role.find_by(name: 'Agent')
-    assert_raises(Exceptions::UnprocessableEntity) do
-      agent_role.default_at_signup = true
-      agent_role.save!
-    end
-
-    admin_role = Role.find_by(name: 'Admin')
-    assert_raises(Exceptions::UnprocessableEntity) do
-      admin_role.default_at_signup = true
-      admin_role.save!
-    end
-
-    assert_raises(Exceptions::UnprocessableEntity) do
-      Role.create!(
-        name:              'Test1',
-        note:              'Test1 Role.',
-        default_at_signup: true,
-        permissions:       [Permission.find_by(name: 'admin')],
-        updated_by_id:     1,
-        created_by_id:     1
-      )
-    end
-
-    role = Role.create!(
-      name:              'Test1',
-      note:              'Test1 Role.',
-      default_at_signup: false,
-      permissions:       [Permission.find_by(name: 'admin')],
-      updated_by_id:     1,
-      created_by_id:     1
-    )
-    assert(role)
-
-    permissions = Permission.where('name LIKE ? OR name = ?', 'admin%', 'ticket.agent').pluck(:name) # get all administrative permissions
-    permissions.each do |type|
-
-      assert_raises(Exceptions::UnprocessableEntity) do
-        Role.create!(
-          name:              "Test1_#{type}",
-          note:              'Test1 Role.',
-          default_at_signup: true,
-          permissions:       [Permission.find_by(name: type)],
-          updated_by_id:     1,
-          created_by_id:     1
-        )
-      end
-
-      role = Role.create!(
-        name:              "Test1_#{type}",
-        note:              'Test1 Role.',
-        default_at_signup: false,
-        permissions:       [Permission.find_by(name: type)],
-        updated_by_id:     1,
-        created_by_id:     1
-      )
-      assert(role)
-    end
-
-    assert_raises(Exceptions::UnprocessableEntity) do
-      Role.create!(
-        name:              'Test2',
-        note:              'Test2 Role.',
-        default_at_signup: true,
-        permissions:       [Permission.find_by(name: 'ticket.agent')],
-        updated_by_id:     1,
-        created_by_id:     1
-      )
-    end
-
-    role = Role.create!(
-      name:              'Test2',
-      note:              'Test2 Role.',
-      default_at_signup: false,
-      permissions:       [Permission.find_by(name: 'ticket.agent')],
-      updated_by_id:     1,
-      created_by_id:     1
-    )
-    assert(role)
-
-  end
-
-end