From dccd4aa012c67368e18ae5553fc0d328a9da8d95 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 27 Nov 2020 14:08:30 +0100 Subject: [PATCH] Fixes #3299 - Having more active Agents using the Chat decreases performance linearly. --- app/models/chat/agent.rb | 6 ++ lib/sessions/event/chat_agent_state.rb | 22 +++-- spec/factories/chat/agent.rb | 1 + .../sessions/event/chat_agent_state_spec.rb | 63 ++++++++++++++ spec/models/chat/agent_spec.rb | 82 +++++++++++++++++++ 5 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 spec/lib/sessions/event/chat_agent_state_spec.rb create mode 100644 spec/models/chat/agent_spec.rb diff --git a/app/models/chat/agent.rb b/app/models/chat/agent.rb index e584c9e94..10bffd07e 100644 --- a/app/models/chat/agent.rb +++ b/app/models/chat/agent.rb @@ -20,10 +20,16 @@ class Chat::Agent < ApplicationModel return chat_agent.active end + + # ATTENTION: setter return value indicates whether `active` state has changed if chat_agent chat_agent.active = state + # always update `updated_at` to inform other Agent sessions + # that this Agent session is still active chat_agent.updated_at = Time.zone.now chat_agent.save + + chat_agent.active_previously_changed? else Chat::Agent.create( active: state, diff --git a/lib/sessions/event/chat_agent_state.rb b/lib/sessions/event/chat_agent_state.rb index e41d5aea7..15c4df6f3 100644 --- a/lib/sessions/event/chat_agent_state.rb +++ b/lib/sessions/event/chat_agent_state.rb @@ -23,14 +23,7 @@ return is sent as message back to peer # check if user has permissions return if !permission_check('chat.agent', 'chat') - chat_user = User.lookup(id: @session['id']) - - Chat::Agent.state(@session['id'], @payload['data']['active']) - - chat_ids = Chat.agent_active_chat_ids(chat_user) - - # broadcast new state to agents - Chat.broadcast_agent_state_update(chat_ids, @session['id']) + update_state { event: 'chat_agent_state', @@ -41,4 +34,17 @@ return is sent as message back to peer } end + private + + def update_state + chat_user = User.lookup(id: @session['id']) + + return if !Chat::Agent.state(@session['id'], @payload['data']['active']) + + chat_ids = Chat.agent_active_chat_ids(chat_user) + + # broadcast new state to agents + Chat.broadcast_agent_state_update(chat_ids, @session['id']) + end + end diff --git a/spec/factories/chat/agent.rb b/spec/factories/chat/agent.rb index 8aca69c8b..296679303 100644 --- a/spec/factories/chat/agent.rb +++ b/spec/factories/chat/agent.rb @@ -1,5 +1,6 @@ FactoryBot.define do factory :'chat/agent' do + active { true } created_by_id { 1 } updated_by_id { 1 } end diff --git a/spec/lib/sessions/event/chat_agent_state_spec.rb b/spec/lib/sessions/event/chat_agent_state_spec.rb new file mode 100644 index 000000000..113de54d5 --- /dev/null +++ b/spec/lib/sessions/event/chat_agent_state_spec.rb @@ -0,0 +1,63 @@ +require 'rails_helper' + +RSpec.describe Sessions::Event::ChatAgentState do + + let(:client_id) { rand(123_456_789) } + let(:chat) { Chat.first } + + let(:user) do + create(:agent, preferences: { + chat: { + active: { + chat.id.to_s => 'on' + } + } + }) + end + + let!(:instance) do + Sessions.create(client_id, { 'id' => user.id }, {}) + Sessions.queue(client_id) + described_class.new( + payload: { + 'data' => { + 'active' => active + }, + }, + user_id: user.id, + client_id: client_id, + clients: {}, + session: { + 'id' => user.id + }, + ) + end + + let(:record) { create(:'chat/agent', updated_by: user) } + + before do + Setting.set('chat', true) + end + + context 'when state changes' do + + let(:active) { !record.active } + + it 'broadcasts agent state update' do + allow(Chat).to receive(:broadcast_agent_state_update) + instance.run + expect(Chat).to have_received(:broadcast_agent_state_update) + end + end + + context "when state doesn't change" do + + let(:active) { record.active } + + it "doesn't broadcasts agent state update" do + allow(Chat).to receive(:broadcast_agent_state_update) + instance.run + expect(Chat).not_to have_received(:broadcast_agent_state_update) + end + end +end diff --git a/spec/models/chat/agent_spec.rb b/spec/models/chat/agent_spec.rb new file mode 100644 index 000000000..cd28875d4 --- /dev/null +++ b/spec/models/chat/agent_spec.rb @@ -0,0 +1,82 @@ +require 'rails_helper' + +RSpec.describe Chat::Agent, type: :model do + + describe '.state' do + + let(:user) { create(:agent) } + + context 'when no record exists for User' do + + it 'returns false' do + expect(described_class.state(1337)).to be(false) + end + end + + context 'when active flag is set to true' do + + before do + create(:'chat/agent', active: true, updated_by: user) + end + + it 'returns true' do + expect(described_class.state(user.id)).to be(true) + end + end + + context 'when active flag is set to false' do + + before do + create(:'chat/agent', active: false, updated_by: user) + end + + it 'returns false' do + expect(described_class.state(user.id)).to be(false) + end + end + + context 'when setting state for not existing record' do + it 'creates a record' do + expect { described_class.state(user.id, true) }.to change { described_class.exists?(updated_by: user) }.from(false).to(true) + end + end + + context 'when setting same state for record' do + + let(:record) { create(:'chat/agent', active: true, updated_by: user) } + + before do + # avoid race condition with same updated_at time + record + travel_to 5.minutes.from_now + end + + it 'updates updated_at timestamp' do + expect { described_class.state(record.updated_by_id, record.active) }.to change { record.reload.updated_at } + end + + it 'returns false' do + expect(described_class.state(record.updated_by_id, record.active)).to eq(false) + end + end + + context 'when setting different state for record' do + + let(:record) { create(:'chat/agent', active: true, updated_by: user) } + + before do + # avoid race condition with same updated_at time + record + travel_to 5.minutes.from_now + end + + it 'updates updated_at timestamp' do + expect { described_class.state(record.updated_by_id, !record.active) }.to change { record.reload.updated_at } + end + + it 'returns true' do + expect(described_class.state(record.updated_by_id, !record.active)).to eq(true) + end + end + end +end