Browse Source

Fixes #3299 - Having more active Agents using the Chat decreases performance linearly.

Thorsten Eckel 4 years ago
parent
commit
dccd4aa012

+ 6 - 0
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,

+ 14 - 8
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

+ 1 - 0
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

+ 63 - 0
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

+ 82 - 0
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