Maintenance: Improved notifications flow for replacement agents.

app/models/transaction/notification.rb

@@ -75,10 +75,11 @@ class Transaction::Notification
     # apply out of office agents
     possible_recipients_additions = Set.new
     possible_recipients.each do |user|
-      recursive_ooo_replacements(
+      ooo_replacements(
         user:         user,
         replacements: possible_recipients_additions,
         reasons:      recipients_reason,
+        ticket:       ticket,
       )
     end
 
@@ -339,26 +340,16 @@ class Transaction::Notification
 
   private
 
-  def recursive_ooo_replacements(user:, replacements:, reasons:, level: 0)
+  def ooo_replacements(user:, replacements:, ticket:, reasons:)
-    if level == 10
-      Rails.logger.warn("Found more than 10 replacement levels for agent #{user}.")
-      return
-    end
-
     replacement = user.out_of_office_agent
+
     return if !replacement
-    # return for already found, added and checked users
+
-    # to prevent re-doing complete lookup paths
+    return if !TicketPolicy.new(replacement, ticket).agent_read_access?
+
     return if !replacements.add?(replacement)
 
     reasons[replacement.id] = 'are the out-of-office replacement of the owner'
-
-    recursive_ooo_replacements(
-      user:         replacement,
-      replacements: replacements,
-      reasons:      reasons,
-      level:        level + 1
-    )
   end
 
   def possible_recipients_of_group(group_id)

app/models/user.rb

@@ -217,10 +217,15 @@ returns
 
 =end
 
-  def out_of_office_agent(loop_user_ids: [])
+  def out_of_office_agent(loop_user_ids: [], stack_depth: 10)
     return if !out_of_office?
     return if out_of_office_replacement_id.blank?
 
+    if stack_depth.zero?
+      Rails.logger.warn("Found more than 10 replacement levels for agent #{self}.")
+      return self
+    end
+
     user = User.find_by(id: out_of_office_replacement_id)
 
     # stop if users are occuring multiple times to prevent endless loops
@@ -228,7 +233,7 @@ returns
 
     loop_user_ids |= [out_of_office_replacement_id]
 
-    ooo_agent = user.out_of_office_agent(loop_user_ids: loop_user_ids)
+    ooo_agent = user.out_of_office_agent(loop_user_ids: loop_user_ids, stack_depth: stack_depth - 1)
     return ooo_agent if ooo_agent.present?
 
     user

spec/factories/user.rb

@@ -58,6 +58,18 @@ FactoryBot.define do
       user.define_singleton_method(:password_plain, -> { password_plain })
     end
 
+    trait :groupable do
+      transient do
+        group { nil }
+      end
+
+      after(:create) do |user, context|
+        Array(context.group).each do |group|
+          user.groups << group
+        end
+      end
+    end
+
     trait :preferencable do
       transient do
         notification_group_ids { [] }
@@ -77,6 +89,17 @@ FactoryBot.define do
         }
       end
     end
+
+    trait :ooo do
+      transient do
+        ooo_agent { nil }
+      end
+
+      out_of_office { true }
+      out_of_office_start_at { 1.day.ago }
+      out_of_office_end_at { 1.day.from_now }
+      out_of_office_replacement_id { ooo_agent.id }
+    end
   end
 
   sequence(:password_valid) do |n|

spec/models/transaction/notification_spec.rb

@@ -31,7 +31,77 @@ RSpec.describe Transaction::Notification, type: :model do
     end
   end
 
+  describe '#ooo_replacements' do
+    subject(:notification_instance) { build(ticket, user) }
+
+    let(:group)         { create(:group) }
+    let(:user)          { create(:agent, :ooo, :groupable, ooo_agent: replacement_1, group: group) }
+    let(:ticket)        { create(:ticket, owner: user, group: group, state_name: 'open', pending_time: Time.current) }
+
+    context 'when replacement has access' do
+      let(:replacement_1) { create(:agent, :groupable, group: group) }
+
+      it 'is added to list' do
+        replacements = Set.new
+
+        ooo(notification_instance, user, replacements: replacements)
+
+        expect(replacements).to include replacement_1
+      end
+
+      context 'when replacement has replacement' do
+        let(:replacement_1) { create(:agent, :ooo, :groupable, ooo_agent: replacement_2, group: group) }
+        let(:replacement_2) { create(:agent, :groupable, group: group) }
+
+        it 'replacement\'s replacement added to list' do
+          replacements = Set.new
+
+          ooo(notification_instance, user, replacements: replacements)
+
+          expect(replacements).to include replacement_2
+        end
+
+        it 'intermediary replacement is not in list' do
+          replacements = Set.new
+
+          ooo(notification_instance, user, replacements: replacements)
+
+          expect(replacements).not_to include replacement_1
+        end
+      end
+    end
+
+    context 'when replacement does not have access' do
+      let(:replacement_1) { create(:agent) }
+
+      it 'is not added to list' do
+        replacements = Set.new
+
+        ooo(notification_instance, user, replacements: replacements)
+
+        expect(replacements).not_to include replacement_1
+      end
+
+      context 'when replacement has replacement with access' do
+        let(:replacement_1) { create(:agent, :ooo, ooo_agent: replacement_2) }
+        let(:replacement_2) { create(:agent, :groupable, group: group) }
+
+        it 'his replacement may be added' do
+          replacements = Set.new
+
+          ooo(notification_instance, user, replacements: replacements)
+
+          expect(replacements).to include replacement_2
+        end
+      end
+    end
+  end
+
   def run(ticket, user, type)
+    build(ticket, user, type).perform
+  end
+
+  def build(ticket, user, type = 'reminder_reached')
     described_class.new(
       object:           ticket.class.name,
       type:             type,
@@ -40,6 +110,10 @@ RSpec.describe Transaction::Notification, type: :model do
       changes:          nil,
       created_at:       Time.current,
       user_id:          user.id
-    ).perform
+    )
+  end
+
+  def ooo(instance, user, replacements: Set.new, reasons: [])
+    instance.send(:ooo_replacements, user: user, replacements: replacements, ticket: ticket, reasons: reasons)
   end
 end

spec/models/user_spec.rb

@@ -248,6 +248,62 @@ RSpec.describe User, type: :model do
             expect(user.out_of_office_agent).to eq(substitute)
           end
         end
+
+        context 'with stack depth exceeding limit' do
+          let(:replacement_chain) do
+            user = create(:agent)
+
+            14
+              .times
+              .each_with_object([user]) do |_, memo|
+                memo << create(:agent, :ooo, ooo_agent: memo.last)
+              end
+              .reverse
+          end
+
+          let(:ids_executed) { [] }
+
+          before do
+            allow_any_instance_of(described_class).to receive(:out_of_office_agent).and_wrap_original do |method, *args| # rubocop:disable RSpec/AnyInstance
+              ids_executed << method.receiver.id
+              method.call(*args)
+            end
+
+            allow(Rails.logger).to receive(:warn)
+          end
+
+          it 'returns the last agent at the limit' do
+            expect(replacement_chain.first.out_of_office_agent).to eq replacement_chain[10]
+          end
+
+          it 'does not evaluate element beyond the limit' do
+            user_beyond_limit = replacement_chain[11]
+
+            replacement_chain.first.out_of_office_agent
+
+            expect(ids_executed).not_to include(user_beyond_limit.id)
+          end
+
+          it 'does evaluate element within the limit' do
+            user_within_limit = replacement_chain[5]
+
+            replacement_chain.first.out_of_office_agent
+
+            expect(ids_executed).to include(user_within_limit.id)
+          end
+
+          it 'logs error below the limit' do
+            replacement_chain.first.out_of_office_agent
+
+            expect(Rails.logger).to have_received(:warn).with(%r{#{Regexp.escape('Found more than 10 replacement levels for agent')}})
+          end
+
+          it 'does not logs warn within the limit' do
+            replacement_chain[10].out_of_office_agent
+
+            expect(Rails.logger).not_to have_received(:warn)
+          end
+        end
       end
     end