From 40fd1de4889584900eeea26514c251ca62fa95f5 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Fri, 14 Apr 2017 21:17:07 -0400 Subject: [PATCH] Account search service refactor (#1791) * Begin coverage for account search service * Coverage for hashtag query * Coverage for calling local vs remote find based on domain presence * Spec to check that exact matches are not duped * Coverage of resolve option * Coverage for account being provided * Start to refactor account search service * Isolate query username and domain methods * Isolate exact_match method * Extract methods for local and remote results * Simplify local vs remote and account isoliation * Extract methods for local and remote results * Simplify de-dupe of exact match * Simplify logic to check for non exact remotes * Cache some methods * Remove nil from exact_match from results array * Return exact matches first * Use find_remote even with no domain Account.find_local is just an alias for Account.find_remote(user, nil) - so we can not bother with the conditional here, and call find_remote directly. --- app/services/account_search_service.rb | 84 ++++++++++++--- spec/services/account_search_service_spec.rb | 104 +++++++++++++++++++ 2 files changed, 176 insertions(+), 12 deletions(-) create mode 100644 spec/services/account_search_service_spec.rb diff --git a/app/services/account_search_service.rb b/app/services/account_search_service.rb index f55439dcb..a9cb85500 100644 --- a/app/services/account_search_service.rb +++ b/app/services/account_search_service.rb @@ -1,26 +1,86 @@ # frozen_string_literal: true class AccountSearchService < BaseService + attr_reader :query, :limit, :resolve, :account + def call(query, limit, resolve = false, account = nil) - return [] if query.blank? || query.start_with?('#') + @query = query + @limit = limit + @resolve = resolve + @account = account - username, domain = query.gsub(/\A@/, '').split('@') - domain = nil if TagManager.instance.local_domain?(domain) + search_service_results + end - if domain.nil? - exact_match = Account.find_local(username) - results = account.nil? ? Account.search_for(username, limit) : Account.advanced_search_for(username, account, limit) + private + + def search_service_results + return [] if query_blank_or_hashtag? + + if resolving_non_matching_remote_account? + [FollowRemoteAccountService.new.call("#{query_username}@#{query_domain}")] else - exact_match = Account.find_remote(username, domain) - results = account.nil? ? Account.search_for("#{username} #{domain}", limit) : Account.advanced_search_for("#{username} #{domain}", account, limit) + search_results_and_exact_match.compact.uniq end + end - results = [exact_match] + results.reject { |a| a.id == exact_match.id } if exact_match + def resolving_non_matching_remote_account? + resolve && !exact_match && !domain_is_local? + end - if resolve && !exact_match && !domain.nil? - results = [FollowRemoteAccountService.new.call("#{username}@#{domain}")] + def search_results_and_exact_match + [exact_match] + search_results.to_a + end + + def query_blank_or_hashtag? + query.blank? || query.start_with?('#') + end + + def split_query_string + @_split_query_string ||= query.gsub(/\A@/, '').split('@') + end + + def query_username + @_query_username ||= split_query_string.first + end + + def query_domain + @_query_domain ||= query_without_split? ? nil : split_query_string.last + end + + def query_without_split? + split_query_string.size == 1 + end + + def domain_is_local? + @_domain_is_local ||= TagManager.instance.local_domain?(query_domain) + end + + def exact_match + @_exact_match ||= Account.find_remote(query_username, query_domain) + end + + def search_results + @_search_results ||= if account + advanced_search_results + else + simple_search_results end + end - results + def advanced_search_results + Account.advanced_search_for(terms_for_query, account, limit) + end + + def simple_search_results + Account.search_for(terms_for_query, limit) + end + + def terms_for_query + if domain_is_local? + query_username + else + "#{query_username} #{query_domain}" + end end end diff --git a/spec/services/account_search_service_spec.rb b/spec/services/account_search_service_spec.rb new file mode 100644 index 000000000..fa421c443 --- /dev/null +++ b/spec/services/account_search_service_spec.rb @@ -0,0 +1,104 @@ +require 'rails_helper' + +describe AccountSearchService do + describe '.call' do + describe 'with a query to ignore' do + it 'returns empty array for missing query' do + results = subject.call('', 10) + + expect(results).to eq [] + end + it 'returns empty array for hashtag query' do + results = subject.call('#tag', 10) + + expect(results).to eq [] + end + end + + describe 'searching for a simple term that is not an exact match' do + it 'does not return a nil entry in the array for the exact match' do + match = Fabricate(:account, username: 'matchingusername') + + results = subject.call('match', 5) + expect(results).to eq [match] + end + end + + describe 'searching local and remote users' do + describe 'when no domain' do + before do + allow(Account).to receive(:find_remote) + allow(Account).to receive(:search_for) + subject.call('one', 10) + end + + it 'uses find_remote with nil domain to look for local accounts' do + expect(Account).to have_received(:find_remote).with('one', nil) + end + + it 'uses search_for to find matches' do + expect(Account).to have_received(:search_for).with('one', 10) + end + end + + describe 'when there is a domain' do + before do + allow(Account).to receive(:find_remote) + end + + it 'uses find_remote to look for remote accounts' do + subject.call('two@example.com', 10) + expect(Account).to have_received(:find_remote).with('two', 'example.com') + end + + describe 'and there is no account provided' do + it 'uses search_for to find matches' do + allow(Account).to receive(:search_for) + subject.call('two@example.com', 10, false, nil) + + expect(Account).to have_received(:search_for).with('two example.com', 10) + end + end + + describe 'and there is an account provided' do + it 'uses advanced_search_for to find matches' do + account = Fabricate(:account) + allow(Account).to receive(:advanced_search_for) + subject.call('two@example.com', 10, false, account) + + expect(Account).to have_received(:advanced_search_for).with('two example.com', account, 10) + end + end + end + end + + describe 'with an exact match' do + it 'returns exact match first, and does not return duplicates' do + partial = Fabricate(:account, username: 'exactness') + exact = Fabricate(:account, username: 'exact') + + results = subject.call('exact', 10) + expect(results.size).to eq 2 + expect(results).to eq [exact, partial] + end + end + + describe 'when there is a domain but no exact match' do + it 'follows the remote account when resolve is true' do + service = double(call: nil) + allow(FollowRemoteAccountService).to receive(:new).and_return(service) + + results = subject.call('newuser@remote.com', 10, true) + expect(service).to have_received(:call).with('newuser@remote.com') + end + + it 'does not follow the remote account when resolve is false' do + service = double(call: nil) + allow(FollowRemoteAccountService).to receive(:new).and_return(service) + + results = subject.call('newuser@remote.com', 10, false) + expect(service).not_to have_received(:call) + end + end + end +end