From b8aeabce9402e81c221c9622a7da399bfee7654a Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Tue, 10 Aug 2021 10:14:30 +0200 Subject: [PATCH] Fixes #3648 - high organization user count relates to bad performance. --- .../controllers/organization_profile.coffee | 53 +++++++++++++------ .../controllers/widget/organization.coffee | 49 ++++++++++++----- .../organization_popover_provider.coffee | 16 ++++++ .../app/models/organization.coffee | 38 +++++++++---- .../views/organization_profile/object.jst.eco | 7 ++- .../app/views/popover/organization.jst.eco | 6 +-- .../app/views/widget/organization.jst.eco | 11 ++-- app/controllers/users_controller.rb | 13 ++--- app/models/organization/assets.rb | 9 ++-- app/models/user/search.rb | 18 +++++-- spec/requests/user_spec.rb | 44 +++++++++++++++ spec/system/organization_profile_spec.rb | 24 +++++++++ spec/system/search_spec.rb | 22 ++++++++ spec/system/ticket/zoom_spec.rb | 24 +++++++++ 14 files changed, 269 insertions(+), 65 deletions(-) create mode 100644 spec/system/organization_profile_spec.rb diff --git a/app/assets/javascripts/app/controllers/organization_profile.coffee b/app/assets/javascripts/app/controllers/organization_profile.coffee index 61e8e6ce2..65db8b5c1 100644 --- a/app/assets/javascripts/app/controllers/organization_profile.coffee +++ b/app/assets/javascripts/app/controllers/organization_profile.coffee @@ -116,6 +116,7 @@ class ActionRow extends App.ControllerObserverActionRow ] class Object extends App.ControllerObserver + memberLimit: 10 model: 'Organization' observe: member_ids: true @@ -130,9 +131,36 @@ class Object extends App.ControllerObserver image_source: true events: + 'click .js-showMoreMembers': 'showMoreMembers' 'focusout [contenteditable]': 'update' + showMoreMembers: (e) -> + @preventDefaultAndStopPropagation(e) + @memberLimit = (parseInt(@memberLimit / 100) + 1) * 100 + @renderMembers() + + renderMembers: -> + elLocal = @el + @organization.members(0, @memberLimit, (users) -> + members = [] + for user in users + el = $('
') + new Member( + object_id: user.id + el: el + ) + members.push el + elLocal.find('.js-userList').html(members) + ) + + if @organization.member_ids.length < @memberLimit + @el.find('.js-showMoreMembers').parent().addClass('hidden') + else + @el.find('.js-showMoreMembers').parent().removeClass('hidden') + render: (organization) => + if organization + @organization = organization # update taskbar with new meta data App.TaskManager.touch(@taskKey) @@ -144,20 +172,24 @@ class Object extends App.ControllerObserver # check if value for _id exists name = attributeName nameNew = name.substr(0, name.length - 3) - if nameNew of organization + if nameNew of @organization name = nameNew # add to show if value exists - if (organization[name] || attributeConfig.tag is 'richtext') && attributeConfig.shown + if (@organization[name] || attributeConfig.tag is 'richtext') && attributeConfig.shown # do not show firstname and lastname / already show via diplayName() if name isnt 'name' organizationData.push attributeConfig - @html App.view('organization_profile/object')( - organization: organization + elLocal = $(App.view('organization_profile/object')( + organization: @organization organizationData: organizationData - ) + )) + + @html elLocal + + @renderMembers() @$('[contenteditable]').ce({ mode: 'textonly' @@ -165,17 +197,6 @@ class Object extends App.ControllerObserver maxlength: 250 }) - # show members - members = [] - for userId in organization.member_ids - el = $('
') - new Member( - object_id: userId - el: el - ) - members.push el - @$('.js-userList').html(members) - update: (e) => name = $(e.target).attr('data-name') value = $(e.target).html() diff --git a/app/assets/javascripts/app/controllers/widget/organization.coffee b/app/assets/javascripts/app/controllers/widget/organization.coffee index 5901dbdc3..dc90b7611 100644 --- a/app/assets/javascripts/app/controllers/widget/organization.coffee +++ b/app/assets/javascripts/app/controllers/widget/organization.coffee @@ -1,6 +1,8 @@ class App.WidgetOrganization extends App.Controller + memberLimit: 10 events: + 'click .js-showMoreMembers': 'showMoreMembers' 'focusout [contenteditable]': 'update' constructor: -> @@ -12,9 +14,35 @@ class App.WidgetOrganization extends App.Controller release: => App.Organization.unsubscribe(@subscribeId) + showMoreMembers: (e) -> + @preventDefaultAndStopPropagation(e) + @memberLimit = (parseInt(@memberLimit / 100) + 1) * 100 + @renderMembers() + + renderMembers: -> + elLocal = @el + @organization.members(0, @memberLimit, (users) -> + members = [] + for user in users + el = $('
') + new Member( + object_id: user.id + el: el + ) + members.push el + elLocal.find('.js-userList').html(members) + ) + + if @organization.member_ids.length < @memberLimit + @el.find('.js-showMoreMembers').parent().addClass('hidden') + else + @el.find('.js-showMoreMembers').parent().removeClass('hidden') + render: (organization) => - if !organization - organization = @u + if organization + @organization = organization + else if !@organization + @organization = @u # get display data organizationData = [] @@ -23,7 +51,7 @@ class App.WidgetOrganization extends App.Controller # check if value for _id exists name = attributeName nameNew = name.substr( 0, name.length - 3 ) - if nameNew of organization + if nameNew of @organization name = nameNew # do not show name since it's already shown via diplayName() @@ -36,25 +64,21 @@ class App.WidgetOrganization extends App.Controller # Always show for these two conditions: # 1. the attribute exists and is not empty # 2. it is a richtext note field - continue if ( !organization[name]? || organization[name] is '' ) && attributeConfig.tag isnt 'richtext' + continue if ( !@organization[name]? || @organization[name] is '' ) && attributeConfig.tag isnt 'richtext' # add to show if all checks passed organizationData.push attributeConfig # insert userData elLocal = $(App.view('widget/organization')( - organization: organization + organization: @organization organizationData: organizationData )) - for user in organization.members - new User( - object_id: user.id - el: elLocal.find('div.userList-row[data-id=' + user.id + ']') - ) - @html elLocal + @renderMembers() + @$('[contenteditable]').ce( mode: 'textonly' multiline: true @@ -71,7 +95,7 @@ class App.WidgetOrganization extends App.Controller org.updateAttributes(data) @log 'notice', 'update', name, value, org -class User extends App.ControllerObserver +class Member extends App.ControllerObserver @extend App.PopoverProvidable @registerPopovers 'User' @@ -80,6 +104,7 @@ class User extends App.ControllerObserver firstname: true lastname: true image: true + active: true render: (user) => @html App.view('organization_profile/member')( diff --git a/app/assets/javascripts/app/lib/app_post/popover_provider/organization_popover_provider.coffee b/app/assets/javascripts/app/lib/app_post/popover_provider/organization_popover_provider.coffee index 7e275de1f..0bc8ee55b 100644 --- a/app/assets/javascripts/app/lib/app_post/popover_provider/organization_popover_provider.coffee +++ b/app/assets/javascripts/app/lib/app_post/popover_provider/organization_popover_provider.coffee @@ -7,4 +7,20 @@ class Organization extends App.SingleObjectPopoverProvider displayTitleUsing: (object) -> object.name + buildHtmlContent: (params) -> + html = super + + params.object.members(0, 10, (users) -> + members = [] + for user in users + el = $('
') + if user.active is false + el.addClass('is-inactive') + el.append(user.displayName()) + members.push el + html.find('.js-userList').html(members) + ) + + html + App.PopoverProvider.registerProvider('Organization', Organization) diff --git a/app/assets/javascripts/app/models/organization.coffee b/app/assets/javascripts/app/models/organization.coffee index c0d91e42e..2189289f7 100644 --- a/app/assets/javascripts/app/models/organization.coffee +++ b/app/assets/javascripts/app/models/organization.coffee @@ -29,16 +29,36 @@ Using **Organisations** you can **group** customers. This has among others two i icon: -> 'organization' - @_fillUp: (data) -> + members: (offset, limit, callback) -> + member_ids = @member_ids.slice(offset, limit) + missing_member_ids = _.filter(member_ids, (id) -> !App.User.findNative(id)) - # add users of organization - if data['member_ids'] - data['members'] = [] - for user_id in data['member_ids'] - if App.User.exists(user_id) - user = App.User.findNative(user_id) - data['members'].push user - data + userResult = -> + users = [] + for user_id in member_ids + user = App.User.find(user_id) + continue if !user + users.push(user) + return users + + return callback(userResult()) if missing_member_ids.length < 1 + + App.Ajax.request( + type: 'POST' + url: "#{@constructor.apiPath}/users/search" + data: JSON.stringify( + query: '*' + ids: missing_member_ids + limit: limit + full: true + ) + processData: true, + success: (data, status, xhr) -> + App.Collection.loadAssets(data.assets) + callback(userResult()) + error: (data, status) -> + callback([]) + ) searchResultAttributes: -> classList = ['organization', 'organization-popover' ] diff --git a/app/assets/javascripts/app/views/organization_profile/object.jst.eco b/app/assets/javascripts/app/views/organization_profile/object.jst.eco index 1c52d89ef..7a0cb0d2d 100644 --- a/app/assets/javascripts/app/views/organization_profile/object.jst.eco +++ b/app/assets/javascripts/app/views/organization_profile/object.jst.eco @@ -17,9 +17,12 @@ <% end %> -<% if @organization.members: %> +<% if @organization.member_ids: %>
+

-<% end %> \ No newline at end of file +<% end %> diff --git a/app/assets/javascripts/app/views/popover/organization.jst.eco b/app/assets/javascripts/app/views/popover/organization.jst.eco index 7bfcd1d5e..a8f40ba47 100644 --- a/app/assets/javascripts/app/views/popover/organization.jst.eco +++ b/app/assets/javascripts/app/views/popover/organization.jst.eco @@ -1,11 +1,9 @@ <%- @V('popover/single_object_generic', object: @object, attributes: @attributes) %> -<% if @object.members: %> +<% if @object.member_ids: %>
- <% for user in @object.members: %> -
<%= user.displayName() %>
- <% end %> +
<% end %> diff --git a/app/assets/javascripts/app/views/widget/organization.jst.eco b/app/assets/javascripts/app/views/widget/organization.jst.eco index 37a6d4629..d6f45f442 100644 --- a/app/assets/javascripts/app/views/widget/organization.jst.eco +++ b/app/assets/javascripts/app/views/widget/organization.jst.eco @@ -25,14 +25,13 @@ <% end %> <% end %> -<% if @organization.members: %> +<% if @organization.member_ids: %>
-
-<% for user in @organization.members: %> -
-<% end %> -
+
+

<% end %> diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ce28c6d91..daaa4ce90 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -217,11 +217,12 @@ class UsersController < ApplicationController # The requester has to be in the role 'Admin' or 'Agent' to # be able to search for User records. # - # @parameter query [String] The search query. - # @parameter limit [Integer] The limit of search results. - # @parameter role_ids(multi) [Array] A list of Role identifiers to which the Users have to be allocated to. - # @parameter group_ids(multi) [HashString,Array>] A list of Group identifiers to which the Users have to be allocated to. - # @parameter full [Boolean] Defines if the result should be + # @parameter query [String] The search query. + # @parameter limit [Integer] The limit of search results. + # @parameter ids(multi) [Array] A list of User IDs which should be returned + # @parameter role_ids(multi) [Array] A list of Role identifiers to which the Users have to be allocated to. + # @parameter group_ids(multi) [HashString,Array>] A list of Group identifiers to which the Users have to be allocated to. + # @parameter full [Boolean] Defines if the result should be # true: { user_ids => [1,2,...], assets => {...} } # or false: [{:id => user.id, :label => "firstname lastname ", :value => "firstname lastname "},...]. # @@ -255,7 +256,7 @@ class UsersController < ApplicationController order_by: params[:order_by], current_user: current_user, } - %i[role_ids group_ids permissions].each do |key| + %i[ids role_ids group_ids permissions].each do |key| next if params[key].blank? query_params[key] = params[key] diff --git a/app/models/organization/assets.rb b/app/models/organization/assets.rb index 0b3371ba7..10fcb9572 100644 --- a/app/models/organization/assets.rb +++ b/app/models/organization/assets.rb @@ -40,11 +40,10 @@ returns app_model_user = User.to_app_model if local_attributes['member_ids'].present? - # feature used for different purpose; do limit references - if local_attributes['member_ids'].count > 100 - local_attributes['member_ids'] = local_attributes['member_ids'].sort[0, 100] - end - local_attributes['member_ids'].each do |local_user_id| + # only provide assets for the first 10 organization users + # rest will be loaded optionally by the frontend + local_attributes['member_ids'] = local_attributes['member_ids'].sort + local_attributes['member_ids'][0, 10].each do |local_user_id| next if data[ app_model_user ] && data[ app_model_user ][ local_user_id ] user = User.lookup(id: local_user_id) diff --git a/app/models/user/search.rb b/app/models/user/search.rb index b9280b1fd..dd4f61081 100644 --- a/app/models/user/search.rb +++ b/app/models/user/search.rb @@ -114,16 +114,20 @@ returns } query_extension['bool']['must'].push access_condition end + + user_ids = [] if params[:group_ids].present? - query_extension['bool'] ||= {} - query_extension['bool']['must'] ||= [] - user_ids = [] params[:group_ids].each do |group_id, access| user_ids |= User.group_access(group_id.to_i, access).pluck(:id) end - return [] if user_ids.blank? - + end + if params[:ids].present? + user_ids |= params[:ids].map(&:to_i) + end + if user_ids.present? + query_extension['bool'] ||= {} + query_extension['bool']['must'] ||= [] query_extension['bool']['must'].push({ 'terms' => { '_id' => user_ids } }) end @@ -149,6 +153,10 @@ returns query.delete! '*' statement = User + if params[:ids].present? + statement = statement.where(id: params[:ids]) + end + if params[:role_ids] statement = statement.joins(:roles).where('roles.id' => params[:role_ids]) end diff --git a/spec/requests/user_spec.rb b/spec/requests/user_spec.rb index 1bc0147da..c074a19f5 100644 --- a/spec/requests/user_spec.rb +++ b/spec/requests/user_spec.rb @@ -1507,6 +1507,50 @@ RSpec.describe 'User', type: :request do end end + describe 'GET /api/v1/users/search, checks usage of the ids parameter', authenticated_as: :agent do + let(:agent) { create(:agent) } + + let(:search_agents) { create_list(:agent, 3, firstname: 'Nick') } + + shared_examples 'ids requests' do + + before do + post '/api/v1/users/search', params: { query: 'Nick', ids: search_ids, sort_by: ['created_at'], order_by: ['ASC'] }, as: :json + end + + shared_examples 'result check' do + + it 'returns only agents matching search parameter ids' do + expect(json_response.map { |row| row['id'] }).to eq(search_ids) + end + end + + context 'when searching for first two agents' do + let(:search_ids) { search_agents.first(2).map(&:id) } + + include_examples 'result check' + end + + context 'when searching for last two agents' do + let(:search_ids) { search_agents.last(2).map(&:id) } + + include_examples 'result check' + end + end + + context 'with elasticsearch', searchindex: true do + include_examples 'ids requests' do + before do + configure_elasticsearch(required: true, rebuild: true) + end + end + end + + context 'without elasticsearch' do + include_examples 'ids requests' + end + end + describe 'PUT /api/v1/users/unlock/{id}' do let(:admin) { create(:admin) } let(:agent) { create(:agent) } diff --git a/spec/system/organization_profile_spec.rb b/spec/system/organization_profile_spec.rb new file mode 100644 index 000000000..820a29529 --- /dev/null +++ b/spec/system/organization_profile_spec.rb @@ -0,0 +1,24 @@ +# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/ + +require 'rails_helper' + +RSpec.describe 'Organization Profile', type: :system do + + context 'members section' do + let(:organization) { create(:organization) } + let(:members) { organization.members.order(id: :asc) } + + before do + create_list(:customer, 50, organization: organization) + visit "#organization/profile/#{organization.id}" + end + + it 'shows first 10 members and loads more on demand' do + expect(page).to have_text(members[9].fullname) + expect(page).to have_no_text(members[10].fullname) + + click '.js-showMoreMembers' + expect(page).to have_text(members[10].fullname) + end + end +end diff --git a/spec/system/search_spec.rb b/spec/system/search_spec.rb index df85a5759..ef19e25b0 100644 --- a/spec/system/search_spec.rb +++ b/spec/system/search_spec.rb @@ -17,6 +17,28 @@ RSpec.describe 'Search', type: :system, searchindex: true do end end + context 'Organization members', authenticated_as: :authenticate do + let(:organization) { create(:organization) } + let(:members) { organization.members.order(id: :asc) } + + def authenticate + create_list(:customer, 50, organization: organization) + true + end + + before do + sleep 3 # wait for popover killer to pass + fill_in id: 'global-search', with: organization.name.to_s + end + + it 'shows only first 10 members' do + expect(page).to have_text(organization.name) + popover_on_hover(first('a.nav-tab.organization')) + expect(page).to have_text(members[9].fullname, wait: 30) + expect(page).to have_no_text(members[10].fullname) + end + end + context 'inactive user and organizations' do before do create(:organization, name: 'Example Inc.', active: true) diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index 14a8b2ed4..78e89034f 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/spec/system/ticket/zoom_spec.rb @@ -1682,4 +1682,28 @@ RSpec.describe 'Ticket zoom', type: :system do expect(page).to have_text(ticket_closed.title, wait: 20) end end + + context 'Sidebar - Organization' do + let(:organization) { create(:organization) } + + context 'members section' do + + let(:customers) { create_list(:customer, 50, organization: organization) } + let(:ticket) { create(:ticket, group: Group.find_by(name: 'Users'), customer: customers.first) } + let(:members) { organization.members.order(id: :asc) } + + before do + visit "#ticket/zoom/#{ticket.id}" + click '.tabsSidebar-tab[data-tab=organization]' + end + + it 'shows first 10 members and loads more on demand' do + expect(page).to have_text(members[9].fullname) + expect(page).to have_no_text(members[10].fullname) + + click '.js-showMoreMembers' + expect(page).to have_text(members[10].fullname) + end + end + end end