From eafb6dd236e5ed4c1cf2b0ac659e5d6ca226a5e7 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Mon, 2 Mar 2020 15:25:33 +0100 Subject: [PATCH] Follow up - ed8a152f28a18e6d070dadbba2e79e524a3b42f7 - Fixes #2911 - Performance: Improved touching assets of users, organizations and tickets. Rollback of indexed data behavior change (structure instead of name). --- .../can_lookup_search_index_attributes.rb | 6 +-- .../concerns/has_search_index_backend.rb | 12 +++-- ...lookup_search_index_attributes_examples.rb | 10 ++-- spec/models/user_spec.rb | 2 + spec/requests/search_spec.rb | 53 +++++++++++-------- test/integration/elasticsearch_test.rb | 2 +- 6 files changed, 47 insertions(+), 38 deletions(-) diff --git a/app/models/application_model/can_lookup_search_index_attributes.rb b/app/models/application_model/can_lookup_search_index_attributes.rb index 80be68498..6d57ffbc5 100644 --- a/app/models/application_model/can_lookup_search_index_attributes.rb +++ b/app/models/application_model/can_lookup_search_index_attributes.rb @@ -94,11 +94,11 @@ returns # get name of ref object value = nil - if respond_to?('search_index_data') + if respond_to?('name') + value = send('name') + elsif respond_to?('search_index_data') value = send('search_index_data') return if value == true - elsif respond_to?('name') - value = send('name') end value diff --git a/app/models/concerns/has_search_index_backend.rb b/app/models/concerns/has_search_index_backend.rb index 8250c5558..99a4d2d34 100644 --- a/app/models/concerns/has_search_index_backend.rb +++ b/app/models/concerns/has_search_index_backend.rb @@ -91,7 +91,9 @@ returns # start background job to transfer data to search index return true if !SearchIndexBackend.enabled? - return if search_index_value.blank? + + new_search_index_value = search_index_value + return if new_search_index_value.blank? Models.indexable.each do |local_object| next if local_object == self.class @@ -106,9 +108,9 @@ returns next if local_object.to_s == 'Ticket' local_object.new.attributes.each do |key, _value| - attribute_name = key.to_s - attribute_ref_name = local_object.search_index_attribute_ref_name(attribute_name) - attribute_class = local_object.reflect_on_association(attribute_ref_name)&.klass + attribute_name = key.to_s + attribute_ref_name = local_object.search_index_attribute_ref_name(attribute_name) + attribute_class = local_object.reflect_on_association(attribute_ref_name)&.klass next if attribute_name.blank? next if attribute_ref_name.blank? @@ -116,7 +118,7 @@ returns next if attribute_class != self.class data = { - attribute_ref_name => search_index_value + attribute_ref_name => new_search_index_value, } where = { attribute_name => id diff --git a/spec/models/user/can_lookup_search_index_attributes_examples.rb b/spec/models/user/can_lookup_search_index_attributes_examples.rb index 0e5b4a26d..d4df02be4 100644 --- a/spec/models/user/can_lookup_search_index_attributes_examples.rb +++ b/spec/models/user/can_lookup_search_index_attributes_examples.rb @@ -1,13 +1,11 @@ RSpec.shared_examples 'CanLookupSearchIndexAttributes' do describe '.search_index_value_by_attribute' do - it 'returns hash of data' do + it 'returns search index value for attribute' do organization = create(:organization, name: 'Tomato42', note: 'special recipe') user = create(:agent_user, organization: organization) value = user.search_index_value_by_attribute('organization_id') - expect_value = { 'name' => 'Tomato42', 'note' => 'special recipe' } - expect(value).to be_a_kind_of(Hash) - expect(value).to eq(expect_value) + expect(value).to eq('Tomato42') end end @@ -16,9 +14,7 @@ RSpec.shared_examples 'CanLookupSearchIndexAttributes' do organization = create(:organization, name: 'Tomato42', note: 'special recipe') value = organization.search_index_value - expect_value = { 'name' => 'Tomato42', 'note' => 'special recipe' } - expect(value).to be_a_kind_of(Hash) - expect(value).to eq(expect_value) + expect(value).to eq('Tomato42') end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fd67de049..154c83f15 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -8,6 +8,7 @@ require 'models/concerns/has_xss_sanitized_note_examples' require 'models/concerns/can_be_imported_examples' require 'models/concerns/has_object_manager_attributes_validation_examples' require 'models/user/has_ticket_create_screen_impact_examples' +require 'models/user/can_lookup_search_index_attributes_examples' RSpec.describe User, type: :model do subject(:user) { create(:user) } @@ -25,6 +26,7 @@ RSpec.describe User, type: :model do it_behaves_like 'CanBeImported' it_behaves_like 'HasObjectManagerAttributesValidation' it_behaves_like 'HasTicketCreateScreenImpact' + it_behaves_like 'CanLookupSearchIndexAttributes' describe 'Class methods:' do describe '.authenticate' do diff --git a/spec/requests/search_spec.rb b/spec/requests/search_spec.rb index dbc65002f..5edd101ea 100644 --- a/spec/requests/search_spec.rb +++ b/spec/requests/search_spec.rb @@ -352,15 +352,18 @@ RSpec.describe 'Search', type: :request, searchindex: true do end it 'does find the user of the nested organization and also even if the organization name changes' do - params = { - query: 'Tomato42', - limit: 10, - } # because of the initial relation between user and organization # both user and organization will be found as result authenticated_as(agent_user) - post '/api/v1/search/User', params: params, as: :json + post '/api/v1/search/User', params: { query: 'Tomato42' }, as: :json + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response).to be_truthy + expect(json_response['assets']['Organization'][organization_nested.id.to_s]).to be_truthy + expect(json_response['assets']['User'][customer_user_nested.id.to_s]).to be_truthy + + post '/api/v1/search/User', params: { query: 'organization:Tomato42' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) expect(json_response).to be_truthy @@ -371,14 +374,16 @@ RSpec.describe 'Search', type: :request, searchindex: true do Scheduler.worker(true) SearchIndexBackend.refresh - params = { - query: 'Cucumber43', - limit: 10, - } - # even after a change of the organization name we should find # the customer user because of the nested organization data - post '/api/v1/search/User', params: params, as: :json + post '/api/v1/search/User', params: { query: 'Cucumber43' }, as: :json + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response).to be_truthy + expect(json_response['assets']['Organization'][organization_nested.id.to_s]).to be_truthy + expect(json_response['assets']['User'][customer_user_nested.id.to_s]).to be_truthy + + post '/api/v1/search/User', params: { query: 'organization:Cucumber43' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) expect(json_response).to be_truthy @@ -387,13 +392,15 @@ RSpec.describe 'Search', type: :request, searchindex: true do end it 'does find the ticket by organization name even if the organization name changes' do - params = { - query: 'Tomato42', - limit: 10, - } - authenticated_as(agent_user) - post '/api/v1/search/Ticket', params: params, as: :json + post '/api/v1/search/Ticket', params: { query: 'Tomato42' }, as: :json + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response).to be_truthy + expect(json_response['assets']['Organization'][organization_nested.id.to_s]).to be_truthy + expect(json_response['assets']['Ticket'][ticket_nested.id.to_s]).to be_truthy + + post '/api/v1/search/Ticket', params: { query: 'organization:Tomato42' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) expect(json_response).to be_truthy @@ -404,12 +411,14 @@ RSpec.describe 'Search', type: :request, searchindex: true do Scheduler.worker(true) SearchIndexBackend.refresh - params = { - query: 'Cucumber43', - limit: 10, - } + post '/api/v1/search/Ticket', params: { query: 'Cucumber43' }, as: :json + expect(response).to have_http_status(:ok) + expect(json_response).to be_a_kind_of(Hash) + expect(json_response).to be_truthy + expect(json_response['assets']['Organization'][organization_nested.id.to_s]).to be_truthy + expect(json_response['assets']['Ticket'][ticket_nested.id.to_s]).to be_truthy - post '/api/v1/search/Ticket', params: params, as: :json + post '/api/v1/search/Ticket', params: { query: 'organization:Cucumber43' }, as: :json expect(response).to have_http_status(:ok) expect(json_response).to be_a_kind_of(Hash) expect(json_response).to be_truthy diff --git a/test/integration/elasticsearch_test.rb b/test/integration/elasticsearch_test.rb index 4591a9cb5..0511bb454 100644 --- a/test/integration/elasticsearch_test.rb +++ b/test/integration/elasticsearch_test.rb @@ -104,7 +104,7 @@ class ElasticsearchTest < ActiveSupport::TestCase assert_equal('es-customer1@example.com', attributes['email']) assert(attributes['preferences']) assert_not(attributes['password']) - assert_equal({ 'name' => 'Customer Organization Update', 'note' => 'some note' }, attributes['organization']) + assert_equal('Customer Organization Update', attributes['organization']) # organization attributes = @organization1.search_index_data