From 15f1c970c844b94a2de33dff65a186ec6c1853d1 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 29 Oct 2021 10:49:17 +0200 Subject: [PATCH] Maintenance: Improved notifications flow for replacement agents. --- app/models/transaction/notification.rb | 23 ++---- app/models/user.rb | 9 ++- spec/factories/user.rb | 23 ++++++ spec/models/transaction/notification_spec.rb | 76 +++++++++++++++++++- spec/models/user_spec.rb | 56 +++++++++++++++ 5 files changed, 168 insertions(+), 19 deletions(-) diff --git a/app/models/transaction/notification.rb b/app/models/transaction/notification.rb index 3e89d8ef5..03964e8ac 100644 --- a/app/models/transaction/notification.rb +++ b/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) - if level == 10 - Rails.logger.warn("Found more than 10 replacement levels for agent #{user}.") - return - end - + def ooo_replacements(user:, replacements:, ticket:, reasons:) 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) diff --git a/app/models/user.rb b/app/models/user.rb index 882590d06..181ab893f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -220,10 +220,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 @@ -231,7 +236,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 diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 14edb9906..cf7af9266 100644 --- a/spec/factories/user.rb +++ b/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| diff --git a/spec/models/transaction/notification_spec.rb b/spec/models/transaction/notification_spec.rb index 7d034314c..7d285dca3 100644 --- a/spec/models/transaction/notification_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d9309fe69..4ca518d0b 100644 --- a/spec/models/user_spec.rb +++ b/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