commit: 40fd1de4889584900eeea26514c251ca62fa95f5
parent: 31f0bcf8045e4502a38c019004abe63786641682
Author: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Fri, 14 Apr 2017 21:17:07 -0400
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.
Diffstat:
2 files changed, 176 insertions(+), 12 deletions(-)
diff --git 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
+
+ search_service_results
+ end
+
+ private
- username, domain = query.gsub(/\A@/, '').split('@')
- domain = nil if TagManager.instance.local_domain?(domain)
+ def search_service_results
+ return [] if query_blank_or_hashtag?
- if domain.nil?
- exact_match = Account.find_local(username)
- results = account.nil? ? Account.search_for(username, limit) : Account.advanced_search_for(username, account, limit)
+ 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
+
+ def resolving_non_matching_remote_account?
+ resolve && !exact_match && !domain_is_local?
+ end
+
+ 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
- results = [exact_match] + results.reject { |a| a.id == exact_match.id } if exact_match
+ 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
- if resolve && !exact_match && !domain.nil?
- results = [FollowRemoteAccountService.new.call("#{username}@#{domain}")]
+ def search_results
+ @_search_results ||= if account
+ advanced_search_results
+ else
+ simple_search_results
end
+ end
+
+ def advanced_search_results
+ Account.advanced_search_for(terms_for_query, account, limit)
+ end
- results
+ 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
@@ -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
