From 4e8cf209cab5e2b679086400c8ecfb8b840687e1 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 28 Nov 2019 07:59:22 +0100 Subject: [PATCH] Performance: Observing every Overview of each Agent Session doesn't scale well on larger systems (60 Overviews per Agent). With this change only the last 5 used Overviews are checked on every iteration. A full check is still performed ever 60 seconds. This reduces the overall load. --- .../app/controllers/ticket_overview.coffee | 6 ++ .../app/controllers/ticket_zoom.coffee | 6 +- .../app/lib/mixins/ticket_navigable.coffee | 24 ++++- app/models/ticket/overviews.rb | 22 ++++- config/environments/development.rb | 22 ++--- lib/sessions/backend/ticket_overview_list.rb | 97 ++++++++++++++++--- lib/sessions/event/ticket_overview_select.rb | 23 +++++ .../backend/ticket_overview_list_spec.rb | 14 +-- test/browser/agent_ticket_link_test.rb | 29 ++++++ test/browser/agent_ticket_macro_test.rb | 8 +- .../browser/agent_ticket_overview_tab_test.rb | 8 +- test/browser_test_helper.rb | 4 +- 12 files changed, 215 insertions(+), 48 deletions(-) create mode 100644 lib/sessions/event/ticket_overview_select.rb diff --git a/app/assets/javascripts/app/controllers/ticket_overview.coffee b/app/assets/javascripts/app/controllers/ticket_overview.coffee index da10558d1..d854b5ad0 100644 --- a/app/assets/javascripts/app/controllers/ticket_overview.coffee +++ b/app/assets/javascripts/app/controllers/ticket_overview.coffee @@ -936,6 +936,10 @@ class Navbar extends App.Controller if item.link is @view @title item.name, true + # send first view info + if !@view && data && data[0] && data[0].link + App.WebSocket.send(event:'ticket_overview_select', data: { view: data[0].link }) + # redirect to first view if @activeState && !@view && !@vertical view = data[0].link @@ -1037,6 +1041,8 @@ class Table extends App.Controller @view_mode = App.LocalStorage.get("mode:#{@view}", @Session.get('id')) || 's' console.log 'notice', 'view:', @view, @view_mode + App.WebSocket.send(event:'ticket_overview_select', data: { view: @view }) + # get ticket list ticketListShow = [] for ticket in tickets diff --git a/app/assets/javascripts/app/controllers/ticket_zoom.coffee b/app/assets/javascripts/app/controllers/ticket_zoom.coffee index 652d078d3..65d3c77ff 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom.coffee @@ -894,6 +894,10 @@ class App.TicketZoom extends App.Controller if macro && macro.ux_flow_next_up taskAction = macro.ux_flow_next_up + nextTicket = undefined + if taskAction is 'closeNextInOverview' || taskAction is 'next_from_overview' + nextTicket = @getNextTicketInOverview() + # submit changes @ajax( id: "ticket_update_#{ticket.id}" @@ -916,8 +920,8 @@ class App.TicketZoom extends App.Controller @sidebarWidget.commit() if taskAction is 'closeNextInOverview' || taskAction is 'next_from_overview' + @openTicketInOverview(nextTicket) App.Event.trigger('overview:fetch') - @taskOpenNextTicketInOverview() return if taskAction is 'closeTab' || taskAction is 'next_task' diff --git a/app/assets/javascripts/app/lib/mixins/ticket_navigable.coffee b/app/assets/javascripts/app/lib/mixins/ticket_navigable.coffee index 2ccf55731..e35f6ff6e 100644 --- a/app/assets/javascripts/app/lib/mixins/ticket_navigable.coffee +++ b/app/assets/javascripts/app/lib/mixins/ticket_navigable.coffee @@ -22,22 +22,40 @@ App.TicketNavigable = show: true ) + getNextTicketInOverview: -> + return if !@ticket + return if !@overview_id + + App.Overview.find(@overview_id).nextTicket(@ticket) + + openTicketInOverview: (nextTicket) -> + if nextTicket + @taskCloseTicket() + @taskLoadTicket(nextTicket.id) + return + + @taskCloseTicket(true) + taskOpenNextTicketInOverview: -> if !(@overview_id? && @ticket?) @taskCloseTicket(true) return - next_ticket = App.Overview.find(@overview_id).nextTicket(@ticket) - if next_ticket + + nextTicket = @getNextTicketInOverview() + if nextTicket @taskCloseTicket() - @taskLoadTicket(next_ticket.id) + @taskLoadTicket(nextTicket.id) return + @taskCloseTicket(true) taskCloseTicket: (openNext = false) -> App.TaskManager.remove(@taskKey) return if !openNext + nextTaskUrl = App.TaskManager.nextTaskUrl() if nextTaskUrl @navigate nextTaskUrl return + @navigate '#' diff --git a/app/models/ticket/overviews.rb b/app/models/ticket/overviews.rb index 29a30b237..7ebc64666 100644 --- a/app/models/ticket/overviews.rb +++ b/app/models/ticket/overviews.rb @@ -7,6 +7,10 @@ all overviews by user result = Ticket::Overviews.all(current_user: User.find(3)) +certain overviews by user + + result = Ticket::Overviews.all(current_user: User.find(3), links: ['all_unassigned', 'my_assigned']) + returns result = [overview1, overview2] @@ -15,6 +19,7 @@ returns def self.all(data) current_user = data[:current_user] + links = data[:links] # get customer overviews role_ids = User.joins(:roles).where(users: { id: current_user.id, active: true }, roles: { active: true }).pluck('roles.id') @@ -23,6 +28,9 @@ returns if current_user.organization_id && current_user.organization.shared overview_filter.delete(:organization_shared) end + if links.present? + overview_filter[:link] = links + end overviews = Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: nil }, overviews: overview_filter).or(Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: current_user.id }, overviews: overview_filter)).distinct('overview.id').order(:prio, :name) return overviews end @@ -35,12 +43,21 @@ returns if User.where('out_of_office = ? AND out_of_office_start_at <= ? AND out_of_office_end_at >= ? AND out_of_office_replacement_id = ? AND active = ?', true, Time.zone.today, Time.zone.today, current_user.id, true).count.positive? overview_filter_not = {} end + if links.present? + overview_filter[:link] = links + end Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: nil }, overviews: overview_filter).or(Overview.joins(:roles).left_joins(:users).where(overviews_roles: { role_id: role_ids }, overviews_users: { user_id: current_user.id }, overviews: overview_filter)).where.not(overview_filter_not).distinct('overview.id').order(:prio, :name) end =begin - result = Ticket::Overviews.index(User.find(123)) +index of all overviews by user + + result = Ticket::Overviews.index(User.find(3)) + +index of certain overviews by user + + result = Ticket::Overviews.index(User.find(3), ['all_unassigned', 'my_assigned']) returns @@ -75,9 +92,10 @@ returns =end - def self.index(user) + def self.index(user, links = nil) overviews = Ticket::Overviews.all( current_user: user, + links: links, ) return [] if overviews.blank? diff --git a/config/environments/development.rb b/config/environments/development.rb index efbef4920..0167a307e 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -12,21 +12,17 @@ Rails.application.configure do # Show full error reports. config.consider_all_requests_local = true - # Commented out to ensure using file cache store as described in config/application.rb # Enable/disable caching. By default caching is disabled. # Run rails dev:cache to toggle caching. - # if Rails.root.join('tmp', 'caching-dev.txt').exist? - # config.action_controller.perform_caching = true - # - # config.cache_store = :memory_store - # config.public_file_server.headers = { - # 'Cache-Control' => "public, max-age=#{2.days.to_i}" - # } - # else - # config.action_controller.perform_caching = false - # - # config.cache_store = :null_store - # end + if Rails.root.join('tmp', 'caching-dev.txt').exist? + config.action_controller.perform_caching = true + + config.public_file_server.headers = { + 'Cache-Control' => "public, max-age=#{2.days.to_i}" + } + else + config.action_controller.perform_caching = false + end # Store uploaded files on the local file system (see config/storage.yml for options) # config.active_storage.service = :local diff --git a/lib/sessions/backend/ticket_overview_list.rb b/lib/sessions/backend/ticket_overview_list.rb index c54983e82..43102e9ce 100644 --- a/lib/sessions/backend/ticket_overview_list.rb +++ b/lib/sessions/backend/ticket_overview_list.rb @@ -4,31 +4,82 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base Cache.write("TicketOverviewPull::#{user_id}", { needed: true }) end - def initialize(user, asset_lookup, client = nil, client_id = nil, ttl = 8) + def initialize(user, asset_lookup, client = nil, client_id = nil, ttl = 7) @user = user @client = client @client_id = client_id @ttl = ttl @asset_lookup = asset_lookup - @last_change = nil + @last_index_lists = nil @last_overview = {} @last_overview_change = nil @last_ticket_change = nil + @last_full_fetch = nil + end + + def self.overview_history_append(overview, user_id) + key = "TicketOverviewHistory::#{user_id}" + history = Cache.get(key) || [] + + history.prepend overview + history.uniq! + if history.count > 4 + history.pop + end + + Cache.write(key, history) + end + + def self.overview_history_get(user_id) + Cache.get("TicketOverviewHistory::#{user_id}") end def load # get whole collection - index_and_lists = Ticket::Overviews.index(@user) + index_and_lists = nil + local_overview_changed = overview_changed? + if !@last_index_lists || !@last_full_fetch || @last_full_fetch < (Time.zone.now.to_i - 60) || local_overview_changed + + # check if min one ticket has changed + return if !ticket_changed?(true) && !local_overview_changed + + index_and_lists = Ticket::Overviews.index(@user) + @last_full_fetch = Time.zone.now.to_i + else + + # check if min one ticket has changed + return if !ticket_changed? && !local_overview_changed + + index_and_lists_local = Ticket::Overviews.index(@user, Sessions::Backend::TicketOverviewList.overview_history_get(@user.id)) + + # compare index_and_lists_local to index_and_lists_local + # return if no changes + + index_and_lists = [] + @last_index_lists.each do |last_index| + found_in_particular_index = false + index_and_lists_local.each do |local_index| + next if local_index[:overview][:id] != last_index[:overview][:id] + + index_and_lists.push local_index + found_in_particular_index = true + break + end + next if found_in_particular_index == true + + index_and_lists.push last_index + end + end # no data exists return if index_and_lists.blank? # no change exists - return if @last_change == index_and_lists + return if @last_index_lists == index_and_lists # remember last state - @last_change = index_and_lists + @last_index_lists = index_and_lists index_and_lists end @@ -36,6 +87,12 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base def local_to_run? return false if !@time_now + return true if pull_overview? + + false + end + + def pull_overview? result = Cache.get("TicketOverviewPull::#{@user.id}") Cache.delete("TicketOverviewPull::#{@user.id}") if result return true if result @@ -48,14 +105,6 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base @time_now = Time.zone.now.to_i - # check if min one ticket or overview has changed - last_overview_change = Overview.latest_change - last_ticket_change = Ticket.latest_change - return if last_ticket_change == @last_ticket_change && last_overview_change == @last_overview_change - - @last_overview_change = last_overview_change - @last_ticket_change = last_ticket_change - # load current data index_and_lists = load return if !index_and_lists @@ -134,4 +183,26 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base nil end + def overview_changed? + + # check if min one overview has changed + last_overview_change = Overview.latest_change + return false if last_overview_change == @last_overview_change + + @last_overview_change = last_overview_change + + true + end + + def ticket_changed?(reset = false) + + # check if min one ticket has changed + last_ticket_change = Ticket.latest_change + return false if last_ticket_change == @last_ticket_change + + @last_ticket_change = last_ticket_change if reset + + true + end + end diff --git a/lib/sessions/event/ticket_overview_select.rb b/lib/sessions/event/ticket_overview_select.rb new file mode 100644 index 000000000..e47b10675 --- /dev/null +++ b/lib/sessions/event/ticket_overview_select.rb @@ -0,0 +1,23 @@ +class Sessions::Event::TicketOverviewSelect < Sessions::Event::Base + +=begin + +Event module to serve spool messages and send them to new client connection. + +To execute this manually, just paste the following into the browser console + + App.WebSocket.send({event:'spool'}) + +=end + + def run + return if @payload['data'].blank? + return if @payload['data']['view'].blank? + return if @session['id'].blank? + + Sessions::Backend::TicketOverviewList.overview_history_append(@payload['data']['view'], @session['id']) + + nil + end + +end diff --git a/spec/lib/sessions/backend/ticket_overview_list_spec.rb b/spec/lib/sessions/backend/ticket_overview_list_spec.rb index ab12394e8..b2b1fa77e 100644 --- a/spec/lib/sessions/backend/ticket_overview_list_spec.rb +++ b/spec/lib/sessions/backend/ticket_overview_list_spec.rb @@ -82,14 +82,14 @@ RSpec.describe Sessions::Backend::TicketOverviewList do let!(:first_call) { collection.push } it 'returns nil' do - expect(collection.push).to be(nil) + expect(collection.push).to eq(nil) end context 'even after the TTL has passed' do before { travel(ttl + 1) } it 'returns nil' do - expect(collection.push).to be(nil) + expect(collection.push).to eq(nil) end end @@ -97,7 +97,7 @@ RSpec.describe Sessions::Backend::TicketOverviewList do before { described_class.reset(admin.id) } it 'returns nil' do - expect(collection.push).to be(nil) + expect(collection.push).to eq(nil) end end end @@ -108,14 +108,14 @@ RSpec.describe Sessions::Backend::TicketOverviewList do context 'before the TTL has passed' do it 'returns nil' do - expect(collection.push).to be(nil) + expect(collection.push).to eq(nil) end context 'after .reset with the user’s id' do before { described_class.reset(admin.id) } it 'returns nil because no ticket and no overview has changed' do - expect(collection.push).to be nil + expect(collection.push).to eq(nil) end end end @@ -124,7 +124,7 @@ RSpec.describe Sessions::Backend::TicketOverviewList do before { travel(ttl + 1) } it 'returns an empty result' do - expect(collection.push).to eq nil + expect(collection.push).to eq(nil) end end @@ -132,7 +132,7 @@ RSpec.describe Sessions::Backend::TicketOverviewList do before { travel(2.hours + 1.second) } it 'returns an empty result' do - expect(collection.push).to eq nil + expect(collection.push).to eq(nil) end end end diff --git a/test/browser/agent_ticket_link_test.rb b/test/browser/agent_ticket_link_test.rb index 709fe7b4f..97d677851 100644 --- a/test/browser/agent_ticket_link_test.rb +++ b/test/browser/agent_ticket_link_test.rb @@ -101,6 +101,35 @@ class AgentTicketLinkTest < TestCase css: '.content.active .ticketLinks', value: ticket2[:title], ) + + # cleanup + ticket_open_by_search( + browser: browser2, + number: ticket1[:number], + ) + sleep 1 + + ticket_update( + browser: browser2, + data: { + state: 'closed', + } + ) + + tasks_close_all() + + ticket_open_by_search( + browser: browser2, + number: ticket2[:number], + ) + sleep 1 + + ticket_update( + browser: browser2, + data: { + state: 'closed', + } + ) end end diff --git a/test/browser/agent_ticket_macro_test.rb b/test/browser/agent_ticket_macro_test.rb index 758b42a17..582de0eb4 100644 --- a/test/browser/agent_ticket_macro_test.rb +++ b/test/browser/agent_ticket_macro_test.rb @@ -15,7 +15,7 @@ class AgentTicketMacroTest < TestCase data: { customer: 'nico', group: 'Users', - title: 'some subject - macro "Close & Tag as Spam" default', + title: 'macro "Close & Tag as Spam" default', body: 'some body - macro "Close & Tag as Spam" default', }, ) @@ -66,7 +66,7 @@ class AgentTicketMacroTest < TestCase data: { customer: 'nico', group: 'Users', - title: "some subject - macro #{macro_name}", + title: "macro #{macro_name}", body: "some body - macro #{macro_name}", }, ) @@ -105,7 +105,7 @@ class AgentTicketMacroTest < TestCase data: { customer: 'nico', group: 'Users', - title: "some subject - macro #{macro_name}", + title: "macro #{macro_name}", body: "some body - macro #{macro_name}", }, ) @@ -134,7 +134,7 @@ class AgentTicketMacroTest < TestCase ux_flow_next_up: ux_flow_next_up, ) - title_prefix = "some subject - macro #{macro_name}" + title_prefix = "macro #{macro_name}" ticket1 = ticket_create( data: { customer: 'nico', diff --git a/test/browser/agent_ticket_overview_tab_test.rb b/test/browser/agent_ticket_overview_tab_test.rb index f12275d5c..a43876899 100644 --- a/test/browser/agent_ticket_overview_tab_test.rb +++ b/test/browser/agent_ticket_overview_tab_test.rb @@ -45,7 +45,6 @@ class AgentTicketOverviewTabTest < TestCase # js: '$(".content.active .sidebar").css("display", "block")', #) #click(text: 'Unassigned & Open') - sleep 8 # till overview is rendered ticket_open_by_overview( number: ticket1[:number], @@ -63,9 +62,10 @@ class AgentTicketOverviewTabTest < TestCase task_type: 'closeNextInOverview', # default: stayOnTab / possible: closeTab, closeNextInOverview, stayOnTab ) - match( - css: '.tasks .task.is-active', - value: "overview tab test #2 - #{title}", + watch_for( + css: '.tasks .task.is-active', + value: "overview tab test #2 - #{title}", + timeout: 8, ) assert_equal(1, @browser.find_elements(css: '.tasks .task').count) diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index b93912e66..e490d6dd1 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -2721,6 +2721,8 @@ wait untill text in selector disabppears end else 6.times do + + # prefere find_elements ofer find_element because of exception handling element = instance.find_elements(partial_link_text: params[:number])[0] break if element @@ -2733,7 +2735,7 @@ wait untill text in selector disabppears end element.click sleep 1 - number = instance.find_elements(css: '.content.active .ticketZoom-header .ticket-number')[0].text + number = instance.find_element(css: '.content.active .ticketZoom-header .ticket-number').text if !number.match?(/#{params[:number]}/) screenshot(browser: instance, comment: 'ticket_open_by_overview_open_failed_failed') raise "unable to open ticket #{params[:number]}!"