From 1c96163adab9f5c02ac904cca672a383920f1f8b Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Wed, 5 Aug 2020 15:48:41 +0200 Subject: [PATCH] Maintenance: Enhance KB global search result scope. --- .rubocop/default.yml | 3 + .../knowledge_base/answer/translation.rb | 22 ++++- app/models/knowledge_base/category.rb | 2 +- .../knowledge_base/category/translation.rb | 2 +- app/models/knowledge_base/search.rb | 15 ++-- app/models/knowledge_base/translation.rb | 2 +- lib/search_knowledge_base_backend.rb | 6 +- spec/factories/knowledge_base/answer.rb | 17 +++- spec/factories/knowledge_base/category.rb | 12 +++ .../lib/search_knowledge_base_backend_spec.rb | 85 +++++++++++++++---- .../knowledge_base/answer/translation_spec.rb | 33 +++++++ spec/models/knowledge_base/category_spec.rb | 30 +++++++ spec/support/knowledge_base_contexts.rb | 8 +- 13 files changed, 200 insertions(+), 37 deletions(-) diff --git a/.rubocop/default.yml b/.rubocop/default.yml index 6b0946d3e..8473df998 100644 --- a/.rubocop/default.yml +++ b/.rubocop/default.yml @@ -292,6 +292,9 @@ Layout/MultilineMethodCallIndentation: Include: - "**/*_spec.rb" +Lint/UnusedMethodArgument: + AllowUnusedKeywordArguments: true + Zammad/PreferNegatedIfOverUnless: Exclude: - 'bin/rspec' diff --git a/app/models/knowledge_base/answer/translation.rb b/app/models/knowledge_base/answer/translation.rb index 1733ca140..147615436 100644 --- a/app/models/knowledge_base/answer/translation.rb +++ b/app/models/knowledge_base/answer/translation.rb @@ -96,13 +96,33 @@ class KnowledgeBase::Answer::Translation < ApplicationModel } end - def search_fallback(query, scope = nil) + def search_es_filter(es_response, _query, kb_locale, options) + return es_response if options[:user]&.permissions?('knowledge_base.editor') + + answer_translations_id = es_response.pluck(:id) + + allowed_answer_translation_ids = KnowledgeBase::Answer + .internal + .joins(:translations) + .where(knowledge_base_answer_translations: { id: answer_translations_id, kb_locale_id: kb_locale.id }) + .pluck('knowledge_base_answer_translations.id') + + es_response.filter { |elem| allowed_answer_translation_ids.include? elem[:id].to_i } + end + + def search_fallback(query, scope = nil, options: {}) fields = %w[title] fields << KnowledgeBase::Answer::Translation::Content.arel_table[:body] output = where_or_cis(fields, query) .joins(:content) + if !options[:user]&.permissions?('knowledge_base.editor') + answer_ids = KnowledgeBase::Answer.internal.pluck(:id) + + output = output.where(answer_id: answer_ids) + end + if scope.present? output = output .joins(:answer) diff --git a/app/models/knowledge_base/category.rb b/app/models/knowledge_base/category.rb index 8397860e9..665fea491 100644 --- a/app/models/knowledge_base/category.rb +++ b/app/models/knowledge_base/category.rb @@ -93,7 +93,7 @@ class KnowledgeBase::Category < ApplicationModel def internal_content?(kb_locale = nil) scope = self_with_children_answers.internal - scope = scope.localed(kb_locale) if kb_locale + scope = scope.localed(kb_locale.system_locale) if kb_locale scope.any? end diff --git a/app/models/knowledge_base/category/translation.rb b/app/models/knowledge_base/category/translation.rb index 8ca1b05ab..d6022c75d 100644 --- a/app/models/knowledge_base/category/translation.rb +++ b/app/models/knowledge_base/category/translation.rb @@ -39,7 +39,7 @@ class KnowledgeBase::Category::Translation < ApplicationModel end class << self - def search_fallback(query, scope = nil) + def search_fallback(query, scope = nil, options: {}) fields = %w[title] output = where_or_cis(fields, query) diff --git a/app/models/knowledge_base/search.rb b/app/models/knowledge_base/search.rb index 8468c2d74..4da87610f 100644 --- a/app/models/knowledge_base/search.rb +++ b/app/models/knowledge_base/search.rb @@ -16,7 +16,8 @@ class KnowledgeBase limit: params[:limit] || 10, from: params[:offset] || 0, sort_by: search_get_sort_by(params, 'updated_at'), - order_by: search_get_order_by(params, 'desc') + order_by: search_get_order_by(params, 'desc'), + user: current_user } kb_locale = KnowledgeBase::Locale.preferred(current_user, KnowledgeBase.first) @@ -33,10 +34,10 @@ class KnowledgeBase def search_es(query, kb_locale, options) options[:query_extension] = { bool: { filter: { term: { kb_locale_id: kb_locale.id } } } } - SearchIndexBackend - .search(query, name, options) - .map { |item| lookup(id: item[:id]) } - .compact + es_response = SearchIndexBackend.search(query, name, options) + es_response = search_es_filter(es_response, query, kb_locale, options) if defined? :search_es_filter + + es_response.map { |item| lookup(id: item[:id]) }.compact end def search_sql(query, kb_locale, options) @@ -46,9 +47,9 @@ class KnowledgeBase # - stip out * we already search for *query* - query.delete! '*' - search_fallback("%#{query}%") + search_fallback("%#{query}%", options: options) .where(kb_locale: kb_locale) - .order(order_sql) + .order(Arel.sql(order_sql)) .offset(options[:from]) .limit(options[:limit]) .to_a diff --git a/app/models/knowledge_base/translation.rb b/app/models/knowledge_base/translation.rb index c4ef045c5..938448af3 100644 --- a/app/models/knowledge_base/translation.rb +++ b/app/models/knowledge_base/translation.rb @@ -28,7 +28,7 @@ class KnowledgeBase::Translation < ApplicationModel end class << self - def search_fallback(query, scope = nil) + def search_fallback(query, scope = nil, options: {}) fields = %w[title] output = where_or_cis(fields, query) diff --git a/lib/search_knowledge_base_backend.rb b/lib/search_knowledge_base_backend.rb index 82f86b70e..91d0360cc 100644 --- a/lib/search_knowledge_base_backend.rb +++ b/lib/search_knowledge_base_backend.rb @@ -24,7 +24,7 @@ class SearchKnowledgeBaseBackend hash end else - search_fallback(query, indexes, user) + search_fallback(query, indexes, { user: user }) end if (limit = @params.fetch(:limit, nil)) @@ -40,10 +40,10 @@ class SearchKnowledgeBaseBackend .flatten end - def search_fallback_for_index(query, index, _options) + def search_fallback_for_index(query, index, options) index .constantize - .search_fallback("%#{query}%", @cached_scope_ids) + .search_fallback("%#{query}%", @cached_scope_ids, options: options) .where(kb_locale: kb_locales) .pluck(:id) .map { |id| { id: id, type: index } } diff --git a/spec/factories/knowledge_base/answer.rb b/spec/factories/knowledge_base/answer.rb index d8c031f53..84dac7754 100644 --- a/spec/factories/knowledge_base/answer.rb +++ b/spec/factories/knowledge_base/answer.rb @@ -3,9 +3,10 @@ FactoryBot.define do transient do add_translation { true } translation_traits { [] } + knowledge_base { nil } end - category { create(:knowledge_base_category) } + category { create(:knowledge_base_category, { knowledge_base: knowledge_base }.compact) } before(:create) do |answer, context| next if answer.translations.present? @@ -13,6 +14,20 @@ FactoryBot.define do answer.translations << build('knowledge_base/answer/translation', *context.translation_traits, answer: answer) end + trait :draft # empty placeholder for better readability + + trait :internal do + internal_at { 1.week.ago } + end + + trait :published do + published_at { 1.week.ago } + end + + trait :archived do + archived_at { 1.week.ago } + end + trait :with_video do transient do translation_traits { [:with_video] } diff --git a/spec/factories/knowledge_base/category.rb b/spec/factories/knowledge_base/category.rb index aee0ff0e4..f802b3baa 100644 --- a/spec/factories/knowledge_base/category.rb +++ b/spec/factories/knowledge_base/category.rb @@ -12,6 +12,18 @@ FactoryBot.define do category.translations << create('knowledge_base/category/translation', category: category) end + + trait :empty # empty placeholder for better readability + + %i[published internal draft archived].each do |state| + trait "containing_#{state}" do + after(:create) do |obj| + create(:knowledge_base_answer, state, parent: obj) + + obj.reload + end + end + end end factory 'kb_category_with_tree', parent: 'knowledge_base/category' do diff --git a/spec/lib/search_knowledge_base_backend_spec.rb b/spec/lib/search_knowledge_base_backend_spec.rb index 676e6dd4d..a928eafed 100644 --- a/spec/lib/search_knowledge_base_backend_spec.rb +++ b/spec/lib/search_knowledge_base_backend_spec.rb @@ -40,25 +40,74 @@ RSpec.describe SearchKnowledgeBaseBackend do end end - context 'with (out) ES is identical' do - [true, false].each do |val| - context "when ES=#{val}", searchindex: val do - before do - if val - configure_elasticsearch(required: true, rebuild: true) do - published_answer - end - else - published_answer - end - end - - let(:first_result) { instance.search(published_answer.translations.first.title, user: user).first } - - it 'ID is an Integer' do - expect(first_result.dig(:id)).to be_a(Integer) - end + context 'with successful API response' do + shared_examples 'verify response' do |elasticsearch:| + it "ID is an Integer when ES=#{elasticsearch}", searchindex: elasticsearch do + published_answer + configure_elasticsearch(required: true, rebuild: true) if elasticsearch + first_result = instance.search(published_answer.translations.first.title, user: user).first + expect(first_result.dig(:id)).to be_a(Integer) end end + + include_examples 'verify response', elasticsearch: true + include_examples 'verify response', elasticsearch: false + end + + context 'with user trait and object state' do + def expected_visibility_instance(ui_identifier) + options = { + knowledge_base: knowledge_base, + locale: primary_locale, + scope: nil, + flavor: ui_identifier + } + + described_class.new options + end + + shared_examples 'verify given search backend' do |permissions:, ui:, elasticsearch:| + is_visible = permissions == :all || permissions == ui + prefix = is_visible ? 'lists' : 'does not list' + + it "#{prefix} in #{ui} interface when ES=#{elasticsearch}", searchindex: elasticsearch do + instance = expected_visibility_instance ui + object + configure_elasticsearch(required: true, rebuild: true) if elasticsearch + expect(instance.search(object.translations.first.title, user: user)).to is_visible ? be_present : be_blank + end + end + + shared_examples 'verify given permissions' do |scope:, trait:, admin:, agent:| + context "with #{trait} #{scope}" do + let(:object) { create("knowledge_base_#{scope}", trait, knowledge_base: knowledge_base) } + + include_examples 'verify given user', user_id: :admin, permissions: admin + include_examples 'verify given user', user_id: :agent, permissions: agent + end + end + + shared_examples 'verify given user' do |user_id:, permissions:| + context "with #{user_id}" do + let(:user) { create(user_id) } + + include_examples 'verify given search backend', permissions: permissions, ui: :agent, elasticsearch: true + include_examples 'verify given search backend', permissions: permissions, ui: :agent, elasticsearch: false + + include_examples 'verify given search backend', permissions: permissions, ui: :public, elasticsearch: true + include_examples 'verify given search backend', permissions: permissions, ui: :public, elasticsearch: false + end + end + + include_examples 'verify given permissions', scope: :answer, trait: :published, admin: :all, agent: :all + include_examples 'verify given permissions', scope: :answer, trait: :internal, admin: :all, agent: :agent + include_examples 'verify given permissions', scope: :answer, trait: :draft, admin: :all, agent: :none + include_examples 'verify given permissions', scope: :answer, trait: :archived, admin: :all, agent: :none + + include_examples 'verify given permissions', scope: :category, trait: :empty, admin: :all, agent: :none + include_examples 'verify given permissions', scope: :category, trait: :containing_published, admin: :all, agent: :all + include_examples 'verify given permissions', scope: :category, trait: :containing_internal, admin: :all, agent: :agent + include_examples 'verify given permissions', scope: :category, trait: :containing_draft, admin: :all, agent: :none + include_examples 'verify given permissions', scope: :category, trait: :containing_archived, admin: :all, agent: :none end end diff --git a/spec/models/knowledge_base/answer/translation_spec.rb b/spec/models/knowledge_base/answer/translation_spec.rb index 21f2c7cdb..94fd98ba0 100644 --- a/spec/models/knowledge_base/answer/translation_spec.rb +++ b/spec/models/knowledge_base/answer/translation_spec.rb @@ -11,4 +11,37 @@ RSpec.describe KnowledgeBase::Answer::Translation, type: :model, current_user_id it { is_expected.to belong_to(:answer) } it { is_expected.to belong_to(:kb_locale) } + + describe '.search' do + include_context 'basic Knowledge Base' + + shared_examples 'verify given search backend' do |trait:, user_id:, is_visible:, elasticsearch:| + prefix = is_visible ? 'lists' : 'does not list' + + it "#{prefix} #{trait} answer to #{user_id} when ES=#{elasticsearch}", searchindex: elasticsearch do + user = create(user_id) + object = create(:knowledge_base_answer, trait, knowledge_base: knowledge_base) + configure_elasticsearch(required: true, rebuild: true) if elasticsearch + + expect(described_class.search({ query: object.translations.first.title, current_user: user })) + .to is_visible ? be_present : be_blank + end + end + + shared_examples 'verify given user' do |trait:, user_id:, is_visible:| + include_examples 'verify given search backend', trait: trait, user_id: user_id, is_visible: is_visible, elasticsearch: true + include_examples 'verify given search backend', trait: trait, user_id: user_id, is_visible: is_visible, elasticsearch: false + end + + shared_examples 'verify given permissions' do |trait:, admin:, agent:, customer:| + include_examples 'verify given user', trait: trait, user_id: :admin, is_visible: admin + include_examples 'verify given user', trait: trait, user_id: :agent, is_visible: agent + include_examples 'verify given user', trait: trait, user_id: :customer, is_visible: customer + end + + include_examples 'verify given permissions', trait: :published, admin: true, agent: true, customer: false + include_examples 'verify given permissions', trait: :internal, admin: true, agent: true, customer: false + include_examples 'verify given permissions', trait: :draft, admin: true, agent: false, customer: false + include_examples 'verify given permissions', trait: :archived, admin: true, agent: false, customer: false + end end diff --git a/spec/models/knowledge_base/category_spec.rb b/spec/models/knowledge_base/category_spec.rb index 7db13e67c..58543b498 100644 --- a/spec/models/knowledge_base/category_spec.rb +++ b/spec/models/knowledge_base/category_spec.rb @@ -94,4 +94,34 @@ RSpec.describe KnowledgeBase::Category, type: :model, current_user_id: 1 do end end end + + describe '#public_content?' do + shared_examples 'verify visibility in given state' do |state:, is_visible:| + it "returns #{is_visible} when contains #{state} answer" do + object = create(:knowledge_base_category, "containing_#{state}") + + expect(object).send is_visible ? :to : :not_to, be_public_content(object.translations.first.kb_locale) + end + end + + include_examples 'verify visibility in given state', state: :published, is_visible: true + include_examples 'verify visibility in given state', state: :internal, is_visible: false + include_examples 'verify visibility in given state', state: :draft, is_visible: false + include_examples 'verify visibility in given state', state: :archived, is_visible: false + end + + describe '#internal_content?' do + shared_examples 'verify visibility in given state' do |state:, is_visible:| + it "returns #{is_visible} when contains #{state} answer" do + object = create(:knowledge_base_category, "containing_#{state}") + + expect(object).send is_visible ? :to : :not_to, be_internal_content(object.translations.first.kb_locale) + end + end + + include_examples 'verify visibility in given state', state: :published, is_visible: true + include_examples 'verify visibility in given state', state: :internal, is_visible: true + include_examples 'verify visibility in given state', state: :draft, is_visible: false + include_examples 'verify visibility in given state', state: :archived, is_visible: false + end end diff --git a/spec/support/knowledge_base_contexts.rb b/spec/support/knowledge_base_contexts.rb index b3ae2bbde..533dfd704 100644 --- a/spec/support/knowledge_base_contexts.rb +++ b/spec/support/knowledge_base_contexts.rb @@ -20,19 +20,19 @@ RSpec.shared_context 'basic Knowledge Base', current_user_id: 1 do end let :published_answer do - create(:knowledge_base_answer, :with_attachment, category: category, published_at: 1.week.ago) + create(:knowledge_base_answer, :published, :with_attachment, category: category) end let :published_answer_with_video do - create(:knowledge_base_answer, :with_video, category: category, published_at: 1.week.ago) + create(:knowledge_base_answer, :published, :with_video, category: category) end let :internal_answer do - create(:knowledge_base_answer, category: category, internal_at: 1.week.ago) + create(:knowledge_base_answer, :internal, category: category) end let :archived_answer do - create(:knowledge_base_answer, category: category, archived_at: 1.week.ago) + create(:knowledge_base_answer, :archived, category: category) end end