From 01717bcc7fadc509e3f9c6a171b3c00fa7f3dc9e Mon Sep 17 00:00:00 2001 From: Billy Zhou Date: Mon, 16 Jul 2018 13:39:24 +0800 Subject: [PATCH] For Elasticsearch, sort by active flag first before everything else --- .gitlab-ci.yml | 2 + lib/search_index_backend.rb | 9 ++ test/integration/elasticsearch_active_test.rb | 125 ++++++++++++++++++ test/integration/elasticsearch_test.rb | 25 ++-- test/support/searchindex_helper.rb | 10 +- 5 files changed, 150 insertions(+), 21 deletions(-) create mode 100644 test/integration/elasticsearch_active_test.rb diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 936727487..7353601b4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -240,6 +240,7 @@ test:integration:es_mysql: - export ES_URL="http://localhost:9200" - rake db:create - rake db:migrate + - ruby -I test/ test/integration/elasticsearch_active_test.rb - ruby -I test/ test/integration/elasticsearch_test.rb - ruby -I test/ test/controllers/search_controller_test.rb - ruby -I test/ test/integration/report_test.rb @@ -259,6 +260,7 @@ test:integration:es_postgresql: - export ES_URL="http://localhost:9200" - rake db:create - rake db:migrate + - ruby -I test/ test/integration/elasticsearch_active_test.rb - ruby -I test/ test/integration/elasticsearch_test.rb - ruby -I test/ test/controllers/search_controller_test.rb - ruby -I test/ test/integration/report_test.rb diff --git a/lib/search_index_backend.rb b/lib/search_index_backend.rb index 626e0d198..7bea80d09 100644 --- a/lib/search_index_backend.rb +++ b/lib/search_index_backend.rb @@ -413,6 +413,15 @@ return search result ) end + # add sorting by active if active is not part of the query + if result.flat_map(&:keys).exclude?(:active) + result.unshift( + active: { + order: 'desc', + }, + ) + end + result.push('_score') result diff --git a/test/integration/elasticsearch_active_test.rb b/test/integration/elasticsearch_active_test.rb new file mode 100644 index 000000000..4d4a940bd --- /dev/null +++ b/test/integration/elasticsearch_active_test.rb @@ -0,0 +1,125 @@ +require 'test_helper' + +class ElasticsearchActiveTest < ActiveSupport::TestCase + include SearchindexHelper + + setup do + + configure_elasticsearch(required: true) + + rebuild_searchindex + + roles = Role.where(name: 'Agent') + groups = Group.where(name: 'Users') + + @agent = User.create!( + login: 'es-agent@example.com', + firstname: 'E', + lastname: 'S', + email: 'es-agent@example.com', + password: 'agentpw', + active: true, + roles: roles, + updated_by_id: 1, + created_by_id: 1, + ) + + roles = Role.where(name: 'Customer') + + (1..6).each do |i| + name = i.even? ? "Active-#{i}" : "Inactive-#{i}" + User.create!( + login: "#{name}-customer#{i}@example.com", + firstname: 'ActiveTest', + lastname: name, + active: i.even?, + roles: roles, + updated_by_id: 1, + created_by_id: 1, + ) + Organization.create!( + name: "TestOrg-#{name}", + active: i.even?, + updated_by_id: 1, + created_by_id: 1, + ) + travel 10.seconds + end + + # execute background jobs to index created/changed objects + Scheduler.worker(true) + sleep 2 # for ES to come ready/indexed + end + + test 'active users appear before inactive users in search results' do + result = User.search( + current_user: @agent, + query: 'ActiveTest', + limit: 15, + ) + assert(result.present?, 'result should not be empty') + + names = result.map(&:lastname) + correct_names = %w[Active-6 Active-4 Active-2 Inactive-5 Inactive-3 Inactive-1] + assert_equal(correct_names, names) + end + + test 'active organizations appear before inactive organizations in search results' do + result = Organization.search( + current_user: @agent, + query: 'TestOrg', + limit: 15, + ) + assert(result.present?, 'result should not be empty') + + names = result.map(&:name) + correct_names = %w[TestOrg-Active-2 + TestOrg-Active-4 + TestOrg-Active-6 + TestOrg-Inactive-1 + TestOrg-Inactive-3 + TestOrg-Inactive-5] + assert_equal(correct_names, names) + end + + test 'ordering of tickets are not affected by the lack of active flags' do + ticket_setup + + result = Ticket.search( + current_user: User.find(1), + query: 'ticket', + limit: 15, + ) + assert(result.present?, 'result should not be empty') + + names = result.map(&:title) + correct_names = %w[Ticket-6 Ticket-5 Ticket-4 Ticket-3 Ticket-2 Ticket-1] + assert_equal(correct_names, names) + end + + def ticket_setup + result = Ticket.search( + current_user: User.find(1), + query: 'ticket', + limit: 15, + ) + return if result.present? + + (1..6).each do |i| + Ticket.create!( + title: "Ticket-#{i}", + group: Group.lookup(name: 'Users'), + customer_id: 1, + state: Ticket::State.lookup(name: 'new'), + priority: Ticket::Priority.lookup(name: '2 normal'), + updated_by_id: 1, + created_by_id: 1, + ) + travel 10.seconds + end + + # execute background jobs to index created/changed objects + Scheduler.worker(true) + sleep 2 # for ES to come ready/indexed + end +end diff --git a/test/integration/elasticsearch_test.rb b/test/integration/elasticsearch_test.rb index a6c5e3fbc..0dff6580e 100644 --- a/test/integration/elasticsearch_test.rb +++ b/test/integration/elasticsearch_test.rb @@ -1,4 +1,4 @@ -require 'integration_test_helper' +require 'test_helper' class ElasticsearchTest < ActiveSupport::TestCase include SearchindexHelper @@ -11,7 +11,7 @@ class ElasticsearchTest < ActiveSupport::TestCase groups = Group.where(name: 'Users') roles = Role.where(name: 'Agent') - @agent = User.create_or_update( + @agent = User.create!( login: 'es-agent@example.com', firstname: 'E', lastname: 'S', @@ -36,7 +36,7 @@ class ElasticsearchTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - @customer1 = User.create_or_update( + @customer1 = User.create!( login: 'es-customer1@example.com', firstname: 'ES', lastname: 'Customer1', @@ -48,8 +48,7 @@ class ElasticsearchTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - sleep 1 - @customer2 = User.create_or_update( + @customer2 = User.create!( login: 'es-customer2@example.com', firstname: 'ES', lastname: 'Customer2', @@ -61,8 +60,7 @@ class ElasticsearchTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) - sleep 1 - @customer3 = User.create_or_update( + @customer3 = User.create!( login: 'es-customer3@example.com', firstname: 'ES', lastname: 'Customer3', @@ -73,6 +71,10 @@ class ElasticsearchTest < ActiveSupport::TestCase updated_by_id: 1, created_by_id: 1, ) + + # execute background jobs to index created/changed objects + Scheduler.worker(true) + end # check search attributes @@ -240,7 +242,7 @@ class ElasticsearchTest < ActiveSupport::TestCase created_by_id: 1, ) ticket1.tag_add('someTagA', 1) - sleep 1 + travel 1.minute ticket2 = Ticket.create!( title: 'something else', @@ -266,7 +268,7 @@ class ElasticsearchTest < ActiveSupport::TestCase created_by_id: 1, ) ticket2.tag_add('someTagB', 1) - sleep 1 + travel 1.minute ticket3 = Ticket.create!( title: 'something else', @@ -293,7 +295,7 @@ class ElasticsearchTest < ActiveSupport::TestCase # execute background jobs Scheduler.worker(true) - sleep 4 + sleep 2 # for ES to come ready/indexed # search as @agent @@ -431,8 +433,7 @@ class ElasticsearchTest < ActiveSupport::TestCase # execute background jobs Scheduler.worker(true) - - sleep 4 + sleep 2 # for ES to come ready/indexed # search for tags result = Ticket.search( diff --git a/test/support/searchindex_helper.rb b/test/support/searchindex_helper.rb index 22151a18b..b6874beeb 100644 --- a/test/support/searchindex_helper.rb +++ b/test/support/searchindex_helper.rb @@ -43,15 +43,7 @@ module SearchindexHelper def rebuild_searchindex Rake::Task.clear Zammad::Application.load_tasks - - if ENV['ES_INDEX_RAND'].blank? - Rake::Task['searchindex:rebuild'].execute - else - # if we have a random index we don't need - # to drop the index in the first place - Rake::Task['searchindex:create'].execute - Rake::Task['searchindex:reload'].execute - end + Rake::Task['searchindex:rebuild'].execute end end