From 7006d573b44db90a0a3de17b74d6de96feefb0ca Mon Sep 17 00:00:00 2001
From: Ryan Lue <rl@zammad.com>
Date: Tue, 15 Jan 2019 13:55:59 +0100
Subject: [PATCH 1/6] Refactoring: Migrate role_test.rb to RSpec.

---
 app/models/permission.rb     |   4 +
 app/models/role.rb           |  34 +++--
 spec/factories/permission.rb |   5 +
 spec/factories/role.rb       |   5 +-
 spec/models/role_spec.rb     | 246 ++++++++++++++++++++++++++++++++---
 test/unit/role_test.rb       | 223 -------------------------------
 6 files changed, 260 insertions(+), 257 deletions(-)
 create mode 100644 spec/factories/permission.rb
 delete mode 100644 test/unit/role_test.rb

diff --git a/app/models/permission.rb b/app/models/permission.rb
index 02dff074e..afe05193f 100644
--- a/app/models/permission.rb
+++ b/app/models/permission.rb
@@ -31,4 +31,8 @@ returns
     names
   end
 
+  def to_s
+    name
+  end
+
 end
diff --git a/app/models/role.rb b/app/models/role.rb
index 013060c81..7cf35f07b 100644
--- a/app/models/role.rb
+++ b/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_update  :validate_permissions, :last_admin_check_by_attribute, :validate_agent_limit_by_attributes, :check_default_at_signup_permissions
+  before_create  :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
-    Rails.logger.debug { "self permission: #{self.permission_ids}" }
-    return true if !self.permission_ids
+  def validate_permissions(permission)
+    Rails.logger.debug { "self permission: #{permission.id}" }
 
-    permission_ids.each do |permission_id|
-      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]
+    raise "Permission #{permission.name} is disabled" if permission.preferences[:disabled]
 
-      permission.preferences[:not].each do |local_permission_name|
-        local_permission = Permission.lookup(name: local_permission_name)
-        next if !local_permission
-        raise "Permission #{permission.name} conflicts with #{local_permission.name}" if permission_ids.include?(local_permission.id)
-      end
-    end
-    true
+    permission.preferences[:not]
+              &.find { |name| name.in?(permissions.map(&:name)) }
+              &.tap { |conflict| raise "Permission #{permission} conflicts with #{conflict}" }
+
+    permissions.find { |p| p.preferences[:not]&.include?(permission.name) }
+               &.tap { |conflict| raise "Permission #{permission} conflicts with #{conflict}" }
   end
 
   def last_admin_check_by_attribute
diff --git a/spec/factories/permission.rb b/spec/factories/permission.rb
new file mode 100644
index 000000000..ab88bc32e
--- /dev/null
+++ b/spec/factories/permission.rb
@@ -0,0 +1,5 @@
+FactoryBot.define do
+  factory :permission do
+    name { Faker::Job.unique.position.downcase }
+  end
+end
diff --git a/spec/factories/role.rb b/spec/factories/role.rb
index 23d0c4463..fe7d9b8e0 100644
--- a/spec/factories/role.rb
+++ b/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
diff --git a/spec/models/role_spec.rb b/spec/models/role_spec.rb
index 8c40fc695..492725e9c 100644
--- a/spec/models/role_spec.rb
+++ b/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
-        User.with_permissions('ticket.agent').count
+    describe 'of "Agent" role:' do
+      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
-        Setting.set('system_agent_limit', current_agent_count)
+      context 'with disabled permission' do
+        let(:permission) { create(:permission, preferences: { disabled: true }) }
 
-        inactive_agent_role = create(:role,
-                                     active:      false,
-                                     permissions: Permission.where(name: 'ticket.agent'))
+        it 'cannot be created' do
+          expect { create(:role, permissions: [permission]) }
+            .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
-          inactive_agent_role.update!(active: true)
-        end.to raise_error(Exceptions::UnprocessableEntity)
+        it 'cannot be created' do
+          expect { create(:role, permissions: [Permission.first, permission]) }
+            .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
diff --git a/test/unit/role_test.rb b/test/unit/role_test.rb
deleted file mode 100644
index 19393c31b..000000000
--- a/test/unit/role_test.rb
+++ /dev/null
@@ -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

From 043a0af9f2655f6daa73066514f047420a2b70b4 Mon Sep 17 00:00:00 2001
From: Ryan Lue <rl@zammad.com>
Date: Tue, 15 Jan 2019 14:11:57 +0100
Subject: [PATCH 2/6] Add Null email driver (support workaround for #224)

---
 .../channel/email_account_overview.jst.eco    |  8 +++++--
 app/models/channel/driver/null.rb             | 22 +++++++++++++++++++
 spec/factories/channel.rb                     | 14 ++++++++++++
 spec/system/admin/channel/email_spec.rb       | 17 ++++++++++++++
 4 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100644 app/models/channel/driver/null.rb
 create mode 100644 spec/system/admin/channel/email_spec.rb

diff --git a/app/assets/javascripts/app/views/channel/email_account_overview.jst.eco b/app/assets/javascripts/app/views/channel/email_account_overview.jst.eco
index 912dd5ac6..cdeb70351 100644
--- a/app/assets/javascripts/app/views/channel/email_account_overview.jst.eco
+++ b/app/assets/javascripts/app/views/channel/email_account_overview.jst.eco
@@ -46,7 +46,9 @@
       <div class="action-block action-block--flex">
         <div class="horizontal">
           <h3><%- @Icon('status', channel.status_in + " inline") %> <%- @T('Inbound') %></h3>
-          <div class="js-editInbound btn btn--text space-left"><%- @T('Edit') %></div>
+          <% if channel.preferences.editable isnt false: %>
+            <div class="js-editInbound btn btn--text space-left"><%- @T('Edit') %></div>
+          <% end %>
         </div>
         <table class="key-value">
           <tr>
@@ -89,7 +91,9 @@
       <div class="action-block action-block--flex">
         <div class="horizontal">
           <h3><%- @Icon('status', channel.status_out + " inline") %> <%- @T('Outbound') %></h3>
-          <div class="js-editOutbound btn btn--text space-left"><%- @T('Edit') %></div>
+          <% if channel.preferences.editable isnt false: %>
+            <div class="js-editOutbound btn btn--text space-left"><%- @T('Edit') %></div>
+          <% end %>
         </div>
         <table class="key-value">
         <% if channel.options.outbound && channel.options.outbound.options: %>
diff --git a/app/models/channel/driver/null.rb b/app/models/channel/driver/null.rb
new file mode 100644
index 000000000..15fe81622
--- /dev/null
+++ b/app/models/channel/driver/null.rb
@@ -0,0 +1,22 @@
+# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
+class Channel::Driver::Null
+  def fetchable?(_channel)
+    false
+  end
+
+  def fetch(*)
+    {
+      result:  'ok',
+      fetched: 0,
+      notice:  '',
+    }
+  end
+
+  def disconnect
+    true
+  end
+
+  def self.streamable?
+    false
+  end
+end
diff --git a/spec/factories/channel.rb b/spec/factories/channel.rb
index 9166b4c2f..2b727e977 100644
--- a/spec/factories/channel.rb
+++ b/spec/factories/channel.rb
@@ -8,6 +8,20 @@ FactoryBot.define do
     updated_by_id 1
     created_by_id 1
 
+    factory :email_channel do
+      area 'Email::Account'
+      options do
+        {
+          inbound:  {
+            adapter: 'null', options: {}
+          },
+          outbound: {
+            adapter: 'sendmail'
+          }
+        }
+      end
+    end
+
     factory :twitter_channel do
       transient do
         custom_options { {} }
diff --git a/spec/system/admin/channel/email_spec.rb b/spec/system/admin/channel/email_spec.rb
new file mode 100644
index 000000000..fecb405da
--- /dev/null
+++ b/spec/system/admin/channel/email_spec.rb
@@ -0,0 +1,17 @@
+require 'rails_helper'
+
+RSpec.describe 'Admin Panel > Channels > Email', type: :system, authenticated: true do
+  # https://github.com/zammad/zammad/issues/224
+  it 'hides "Edit" links when Channel#preferences[:editable] == false' do
+    # ensure that the only existing email channel
+    # has preferences == { editable: false }
+    Channel.destroy_all
+    create(:email_channel, preferences: { editable: false })
+
+    visit '/#channels/email'
+    expect(page).to have_css('#c-account h3', text: 'Inbound')  # Wait for frontend to load
+    expect(page).to have_css('#c-account h3', text: 'Outbound') # Wait for frontend to load
+
+    expect(page).not_to have_css('.js-editInbound, .js-editOutbound', text: 'Edit')
+  end
+end

From dbf24b2f6c77eb071b8efdb57bc4552e6be01508 Mon Sep 17 00:00:00 2001
From: Ryan Lue <hello@ryanlue.com>
Date: Mon, 14 Jan 2019 12:11:50 +0800
Subject: [PATCH 3/6] Refactoring: Migrate cache_test.rb to RSpec

---
 spec/lib/cache_spec.rb  | 94 +++++++++++++++++++++++++++++++++++++++++
 test/unit/cache_test.rb | 57 -------------------------
 2 files changed, 94 insertions(+), 57 deletions(-)
 create mode 100644 spec/lib/cache_spec.rb
 delete mode 100644 test/unit/cache_test.rb

diff --git a/spec/lib/cache_spec.rb b/spec/lib/cache_spec.rb
new file mode 100644
index 000000000..e54208ca2
--- /dev/null
+++ b/spec/lib/cache_spec.rb
@@ -0,0 +1,94 @@
+require 'rails_helper'
+
+RSpec.describe Cache do
+  describe '.get' do
+    before { allow(Rails.cache).to receive(:read) }
+
+    it 'wraps Rails.cache.read' do
+      Cache.get('foo')
+
+      expect(Rails.cache).to have_received(:read).with('foo')
+    end
+
+    context 'with a non-string argument' do
+      it 'passes a string' do
+        Cache.get(:foo)
+
+        expect(Rails.cache).to have_received(:read).with('foo')
+      end
+    end
+  end
+
+  describe '.write' do
+    it 'stores string values' do
+      expect { Cache.write('123', 'some value') }
+        .to change { Cache.get('123') }.to('some value')
+    end
+
+    it 'stores hash values' do
+      expect { Cache.write('123', { key: 'some value' }) }
+        .to change { Cache.get('123') }.to({ key: 'some value' })
+    end
+
+    it 'overwrites previous values' do
+      Cache.write('123', 'some value')
+
+      expect { Cache.write('123', { key: 'some value' }) }
+        .to change { Cache.get('123') }.to({ key: 'some value' })
+    end
+
+    it 'stores hash values with non-ASCII content' do
+      expect { Cache.write('123', { key: 'some valueöäüß' }) }
+        .to change { Cache.get('123') }.to({ key: 'some valueöäüß' })
+    end
+
+    it 'defaults to expires_in: 7.days' do
+      Cache.write('123', 'some value')
+
+      expect { travel 7.days }.not_to change { Cache.get('123') }
+      expect { travel 1.second }.to change { Cache.get('123') }.to(nil)
+    end
+
+    it 'accepts a custom :expires_in option' do
+      Cache.write('123', 'some value', expires_in: 3.seconds)
+
+      expect { travel 4.seconds }.to change { Cache.get('123') }.to(nil)
+    end
+  end
+
+  describe '.delete' do
+    it 'deletes stored values' do
+      Cache.write('123', 'some value')
+
+      expect { Cache.delete('123') }
+        .to change { Cache.get('123') }.to(nil)
+    end
+
+    it 'is idempotent' do
+      Cache.write('123', 'some value')
+      Cache.delete('123')
+
+      expect { Cache.delete('123') }.not_to raise_error
+    end
+  end
+
+  describe '.clear' do
+    it 'deletes all stored values' do
+      Cache.write('123', 'some value')
+      Cache.write('456', 'some value')
+
+      # rubocop:disable Layout/MultilineMethodCallIndentation
+      expect { Cache.clear }
+        .to change { Cache.get('123') }.to(nil)
+        .and change { Cache.get('456') }.to(nil)
+      # rubocop:enable Layout/MultilineMethodCallIndentation
+    end
+
+    it 'is idempotent' do
+      Cache.write('123', 'some value')
+      Cache.clear
+
+      expect { Cache.delete('123') }.not_to raise_error
+    end
+  end
+end
diff --git a/test/unit/cache_test.rb b/test/unit/cache_test.rb
deleted file mode 100644
index d1a0facda..000000000
--- a/test/unit/cache_test.rb
+++ /dev/null
@@ -1,57 +0,0 @@
-require 'test_helper'
-
-class CacheTest < ActiveSupport::TestCase
-  test 'cache' do
-
-    # test 1
-    Cache.write('123', 'some value')
-    cache = Cache.get('123')
-    assert_equal(cache, 'some value')
-
-    Cache.write('123', { key: 'some value' })
-    cache = Cache.get('123')
-    assert_equal(cache, { key: 'some value' })
-
-    # test 2
-    Cache.write('123', { key: 'some valueöäüß' })
-    cache = Cache.get('123')
-    assert_equal(cache, { key: 'some valueöäüß' })
-
-    # test 3
-    Cache.delete('123')
-    cache = Cache.get('123')
-    assert_nil(cache)
-
-    # test 4
-    Cache.write('123', { key: 'some valueöäüß2' })
-    cache = Cache.get('123')
-    assert_equal(cache, { key: 'some valueöäüß2' })
-
-    Cache.delete('123')
-    cache = Cache.get('123')
-    assert_nil(cache)
-
-    # test 5
-    Cache.clear
-    cache = Cache.get('123')
-    assert_nil(cache)
-
-    Cache.delete('123')
-    cache = Cache.get('123')
-    assert_nil(cache)
-
-    # test 6
-    Cache.write('123', { key: 'some valueöäüß2' }, expires_in: 3.seconds)
-    travel 5.seconds
-    cache = Cache.get('123')
-    assert_nil(cache)
-  end
-
-  # verify if second cache write overwrite first one
-  test 'cache reset' do
-    Cache.write('some_reset_key', 123)
-    Cache.write('some_reset_key', 12_356)
-    cache = Cache.get('some_reset_key')
-    assert_equal(cache, 12_356, 'verify')
-  end
-end

From 0e6a6ff1fdb704f634a0c4bf10ab5b00022c9c81 Mon Sep 17 00:00:00 2001
From: Ryan Lue <hello@ryanlue.com>
Date: Mon, 14 Jan 2019 12:13:40 +0800
Subject: [PATCH 4/6] Remove outdated edge case handling from Cache class

---
 lib/cache.rb | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/lib/cache.rb b/lib/cache.rb
index 5ed764832..39e2fe556 100644
--- a/lib/cache.rb
+++ b/lib/cache.rb
@@ -25,18 +25,14 @@ write a cache
 =end
 
   def self.write(key, data, params = {})
-    if !params[:expires_in]
-      params[:expires_in] = 7.days
-    end
+    params[:expires_in] ||= 7.days
 
     # in certain cases, caches are deleted by other thread at same
     # time, just log it
-    begin
-      Rails.cache.write(key.to_s, data, params)
-    rescue => e
-      Rails.logger.error "Can't write cache #{key}: #{e.inspect}"
-      Rails.logger.error e
-    end
+    Rails.cache.write(key.to_s, data, params)
+  rescue => e
+    Rails.logger.error "Can't write cache #{key}: #{e.inspect}"
+    Rails.logger.error e
   end
 
 =begin
@@ -60,9 +56,6 @@ clear whole cache store
 =end
 
   def self.clear
-    # workaround, set test cache before clear whole cache, Rails.cache.clear complains about not existing cache dir
-    Cache.write('test', 1)
-
     Rails.cache.clear
   end
 end

From f84408dfc32b4219296f0a1e0699d44bf59617c8 Mon Sep 17 00:00:00 2001
From: Ryan Lue <hello@ryanlue.com>
Date: Mon, 14 Jan 2019 12:13:40 +0800
Subject: [PATCH 5/6] Support clearer layout of '.and change(...).to(...)' in
 RSpec files

---
 .rubocop.yml                 | 12 ++++++++++--
 spec/lib/cache_spec.rb       |  2 --
 spec/models/http_log_spec.rb |  2 --
 spec/models/user_spec.rb     |  2 --
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/.rubocop.yml b/.rubocop.yml
index 6b00d273d..cc625b5b5 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -234,8 +234,8 @@ Style/NumericPredicate:
 
 Lint/AmbiguousBlockAssociation:
   Description: >-
-                 Checks for ambiguous block association with method when param passed without
-                 parentheses.
+                 Checks for ambiguous block association with method when param
+                 passed without parentheses.
   StyleGuide: '#syntax'
   Enabled: true
   Exclude:
@@ -243,6 +243,14 @@ Lint/AmbiguousBlockAssociation:
     - "**/*_spec.rb"
     - "**/*_examples.rb"
 
+Layout/MultilineMethodCallIndentation:
+  Description: >-
+                 Checks the indentation of the method name part in method calls
+                 that span more than one line.
+  EnforcedStyle: indented
+  Include:
+    - "**/*_spec.rb"
+
 # Special exceptions
 
 Style/HashSyntax:
diff --git a/spec/lib/cache_spec.rb b/spec/lib/cache_spec.rb
index e54208ca2..07041c913 100644
--- a/spec/lib/cache_spec.rb
+++ b/spec/lib/cache_spec.rb
@@ -77,11 +77,9 @@ RSpec.describe Cache do
       Cache.write('123', 'some value')
       Cache.write('456', 'some value')
 
-      # rubocop:disable Layout/MultilineMethodCallIndentation
       expect { Cache.clear }
         .to change { Cache.get('123') }.to(nil)
         .and change { Cache.get('456') }.to(nil)
-      # rubocop:enable Layout/MultilineMethodCallIndentation
     end
 
     it 'is idempotent' do
diff --git a/spec/models/http_log_spec.rb b/spec/models/http_log_spec.rb
index 3fff8aaa9..a5b82c9a6 100644
--- a/spec/models/http_log_spec.rb
+++ b/spec/models/http_log_spec.rb
@@ -9,11 +9,9 @@ RSpec.describe HttpLog do
       subject.request[:content]  = 'foo'.force_encoding('ascii-8bit')
       subject.response[:content] = 'bar'.force_encoding('ascii-8bit')
 
-      # rubocop:disable Layout/MultilineMethodCallIndentation
       expect { subject.save }
         .to change { subject.request[:content].encoding.name }.from('ASCII-8BIT').to('UTF-8')
         .and change { subject.response[:content].encoding.name }.from('ASCII-8BIT').to('UTF-8')
-      # rubocop:enable Layout/MultilineMethodCallIndentation
     end
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 71c01815c..e82221753 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -111,11 +111,9 @@ RSpec.describe User do
         before { user } # create user
 
         it 'replaces CallerId record' do
-          # rubocop:disable Layout/MultilineMethodCallIndentation
           expect { user.update(phone: new_number) }
             .to change { Cti::CallerId.where(caller_id: orig_number).count }.by(-1)
             .and change { Cti::CallerId.where(caller_id: new_number).count }.by(1)
-          # rubocop:enable Layout/MultilineMethodCallIndentation
         end
       end
     end

From 2d5ec5d4851bd44a7f10403fee204ffbe20e36cd Mon Sep 17 00:00:00 2001
From: Ryan Lue <hello@ryanlue.com>
Date: Tue, 15 Jan 2019 17:12:44 +0800
Subject: [PATCH 6/6] Add test case for removed edge case handling in
 Cache.clear

---
 spec/lib/cache_spec.rb | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/spec/lib/cache_spec.rb b/spec/lib/cache_spec.rb
index 07041c913..3db7df3f9 100644
--- a/spec/lib/cache_spec.rb
+++ b/spec/lib/cache_spec.rb
@@ -86,7 +86,15 @@ RSpec.describe Cache do
       Cache.write('123', 'some value')
       Cache.clear
 
-      expect { Cache.delete('123') }.not_to raise_error
+      expect { Cache.clear }.not_to raise_error
+    end
+
+    context 'when cache directory is not present on disk' do
+      before { FileUtils.rm_rf(Rails.cache.cache_path) }
+
+      it 'does not raise an error' do
+        expect { Cache.clear }.not_to raise_error
+      end
     end
   end
 end