From c064a0fa07d6e9057ec056dd583a1dbd318b1a11 Mon Sep 17 00:00:00 2001
From: Justin Coyne <jcoyne@justincoyne.com>
Date: Tue, 31 Jan 2017 16:49:49 -0600
Subject: [PATCH] Refactor SearchHelper module to SearchService class

---
 .rubocop_todo.yml                             |   2 -
 app/controllers/concerns/blacklight/base.rb   |   1 -
 .../concerns/blacklight/bookmarks.rb          |  10 +-
 .../concerns/blacklight/catalog.rb            |  45 ++++-
 .../concerns/blacklight/request_builders.rb   |  35 ----
 .../concerns/blacklight/search_context.rb     |   2 +-
 .../concerns/blacklight/search_helper.rb      | 155 ----------------
 app/services/blacklight/search_service.rb     | 120 +++++++++++++
 lib/blacklight/search_state.rb                |   5 +
 spec/controllers/catalog_controller_spec.rb   |  87 +++++++--
 .../blacklight/search_service_spec.rb}        | 167 ++++++------------
 .../_paginate_compact.html.erb_spec.rb        |  25 +--
 12 files changed, 297 insertions(+), 357 deletions(-)
 delete mode 100644 app/controllers/concerns/blacklight/search_helper.rb
 create mode 100644 app/services/blacklight/search_service.rb
 rename spec/{controllers/blacklight/search_helper_spec.rb => services/blacklight/search_service_spec.rb} (66%)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index b007cb013b..c6d922a902 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -125,7 +125,6 @@ Style/Documentation:
 # SupportedStyles: leading, trailing
 Style/DotPosition:
   Exclude:
-    - 'app/controllers/concerns/blacklight/search_helper.rb'
     - 'lib/blacklight/search_builder.rb'
 
 # Offense count: 1
@@ -240,7 +239,6 @@ Style/MultilineIfModifier:
 # SupportedStyles: aligned, indented, indented_relative_to_receiver
 Style/MultilineMethodCallIndentation:
   Exclude:
-    - 'app/controllers/concerns/blacklight/search_helper.rb'
     - 'lib/blacklight/search_builder.rb'
 
 # Offense count: 12
diff --git a/app/controllers/concerns/blacklight/base.rb b/app/controllers/concerns/blacklight/base.rb
index 3d054e694b..79a19cb93f 100644
--- a/app/controllers/concerns/blacklight/base.rb
+++ b/app/controllers/concerns/blacklight/base.rb
@@ -3,6 +3,5 @@ module Blacklight::Base
   extend ActiveSupport::Concern
 
   include Blacklight::Configurable
-  include Blacklight::SearchHelper
   include Blacklight::SearchContext
 end
diff --git a/app/controllers/concerns/blacklight/bookmarks.rb b/app/controllers/concerns/blacklight/bookmarks.rb
index f797c4c07d..b35f975284 100644
--- a/app/controllers/concerns/blacklight/bookmarks.rb
+++ b/app/controllers/concerns/blacklight/bookmarks.rb
@@ -8,7 +8,6 @@ module Blacklight::Bookmarks
     ##
     # Give Bookmarks access to the CatalogController configuration
     include Blacklight::Configurable
-    include Blacklight::SearchHelper
     include Blacklight::TokenBasedUser
 
     copy_blacklight_config_from(CatalogController)
@@ -25,7 +24,7 @@ module Blacklight::Bookmarks
   def action_documents
     bookmarks = token_or_current_or_guest_user.bookmarks
     bookmark_ids = bookmarks.collect { |b| b.document_id.to_s }
-    fetch(bookmark_ids)
+    search_service.fetch(bookmark_ids, search_state.page_params)
   end
 
   def action_success_redirect_path
@@ -41,8 +40,7 @@ def search_action_url *args
   def index
     @bookmarks = token_or_current_or_guest_user.bookmarks
     bookmark_ids = @bookmarks.collect { |b| b.document_id.to_s }
-
-    @response, deprecated_document_list = fetch(bookmark_ids)
+    @response, deprecated_document_list = search_service.fetch(bookmark_ids, search_state.page_params)
     @document_list = ActiveSupport::Deprecation::DeprecatedObjectProxy.new(deprecated_document_list, "The @document_list instance variable is now deprecated and will be removed in Blacklight 8.0")
 
     respond_to do |format|
@@ -136,6 +134,10 @@ def clear
 
   protected
 
+  def search_service
+    Blacklight::SearchService.new(blacklight_config)
+  end
+
   def verify_user
     unless current_or_guest_user || (action == "index" && token_or_current_or_guest_user)
       flash[:notice] = I18n.t('blacklight.bookmarks.need_login')
diff --git a/app/controllers/concerns/blacklight/catalog.rb b/app/controllers/concerns/blacklight/catalog.rb
index 577512e6a7..45d6c10370 100644
--- a/app/controllers/concerns/blacklight/catalog.rb
+++ b/app/controllers/concerns/blacklight/catalog.rb
@@ -9,7 +9,7 @@ module Blacklight::Catalog
   # The following code is executed when someone includes blacklight::catalog in their
   # own controller.
   included do
-    helper_method :sms_mappings, :has_search_parameters?
+    helper_method :sms_mappings, :has_search_parameters?, :facet_limit_for
 
     helper Blacklight::Facet
 
@@ -22,7 +22,7 @@ module Blacklight::Catalog
 
     # get search results from the solr index
     def index
-      (@response, deprecated_document_list) = search_results(params)
+      (@response, deprecated_document_list) = search_service.search_results(params)
 
       @document_list = ActiveSupport::Deprecation::DeprecatedObjectProxy.new(deprecated_document_list, 'The @document_list instance variable is deprecated; use @response.documents instead.')
 
@@ -43,7 +43,7 @@ def index
     # get a single document from the index
     # to add responses for formats other than html or json see _Blacklight::Document::Export_
     def show
-      deprecated_response, @document = fetch params[:id]
+      deprecated_response, @document = search_service.fetch(params[:id])
       @response = ActiveSupport::Deprecation::DeprecatedObjectProxy.new(deprecated_response, 'The @response instance variable is deprecated; use @document.response instead.')
 
       respond_to do |format|
@@ -70,7 +70,7 @@ def track
     # displays values and pagination links for a single facet field
     def facet
       @facet = blacklight_config.facet_fields[params[:id]]
-      @response = get_facet_field_response(@facet.key, params)
+      @response = search_service.facet_field_response(@facet.key, params)
       @display_facet = @response.aggregations[@facet.key]
       @pagination = facet_paginator(@facet, @display_facet)
       respond_to do |format|
@@ -86,7 +86,7 @@ def facet
     def opensearch
       respond_to do |format|
         format.xml { render layout: false }
-        format.json { render json: get_opensearch_response }
+        format.json { render json: search_service.opensearch_response(params) }
       end
     end
 
@@ -99,7 +99,7 @@ def suggest
     end
 
     def action_documents
-      fetch(Array(params[:id]))
+      search_service.fetch(Array(params[:id]))
     end
 
     def action_success_redirect_path
@@ -113,12 +113,43 @@ def has_search_parameters?
       !params[:q].blank? || !params[:f].blank? || !params[:search_field].blank?
     end
 
+    DEFAULT_FACET_LIMIT = 10
+
+    # Look up facet limit for given facet_field. Will look at config, and
+    # if config is 'true' will look up from Solr @response if available. If
+    # no limit is avaialble, returns nil. Used from #add_facetting_to_solr
+    # to supply f.fieldname.facet.limit values in solr request (no @response
+    # available), and used in display (with @response available) to create
+    # a facet paginator with the right limit.
+    def facet_limit_for(facet_field)
+      facet = blacklight_config.facet_fields[facet_field]
+      return if facet.blank?
+
+      if facet.limit && @response && @response.aggregations[facet_field]
+        limit = @response.aggregations[facet_field].limit
+
+        if limit.nil? # we didn't get or a set a limit, so infer one.
+          facet.limit if facet.limit != true
+        elsif limit == -1 # limit -1 is solr-speak for unlimited
+          nil
+        else
+          limit.to_i - 1 # we added 1 to find out if we needed to paginate
+        end
+      elsif facet.limit
+        facet.limit == true ? DEFAULT_FACET_LIMIT : facet.limit
+      end
+    end
+
     protected
 
     #
     # non-routable methods ->
     #
 
+    def search_service
+      Blacklight::SearchService.new(blacklight_config)
+    end
+
     ##
     # If the params specify a view, then store it in the session. If the params
     # do not specifiy the view, set the view parameter to the value stored in the
@@ -247,7 +278,7 @@ def start_new_search_session?
     end
 
     def suggestions_service
-      Blacklight::SuggestSearch.new(params, repository).suggestions
+      Blacklight::SuggestSearch.new(params, search_service.repository).suggestions
     end
 
     def determine_layout
diff --git a/app/controllers/concerns/blacklight/request_builders.rb b/app/controllers/concerns/blacklight/request_builders.rb
index 0b58b5b089..0025658b34 100644
--- a/app/controllers/concerns/blacklight/request_builders.rb
+++ b/app/controllers/concerns/blacklight/request_builders.rb
@@ -1,14 +1,6 @@
 # frozen_string_literal: true
 module Blacklight
   module RequestBuilders
-    extend ActiveSupport::Concern
-
-    included do
-      if respond_to?(:helper_method)
-        helper_method(:facet_limit_for)
-      end
-    end
-
     # Override this method to use a search builder other than the one in the config
     delegate :search_builder_class, to: :blacklight_config
 
@@ -43,32 +35,5 @@ def previous_and_next_document_params(index, window = 1)
       solr_params[:facet] = false
       solr_params
     end
-
-    DEFAULT_FACET_LIMIT = 10
-
-    # Look up facet limit for given facet_field. Will look at config, and
-    # if config is 'true' will look up from Solr @response if available. If
-    # no limit is avaialble, returns nil. Used from #add_facetting_to_solr
-    # to supply f.fieldname.facet.limit values in solr request (no @response
-    # available), and used in display (with @response available) to create
-    # a facet paginator with the right limit.
-    def facet_limit_for(facet_field)
-      facet = blacklight_config.facet_fields[facet_field]
-      return if facet.blank?
-
-      if facet.limit && @response && @response.aggregations[facet_field]
-        limit = @response.aggregations[facet_field].limit
-
-        if limit.nil? # we didn't get or a set a limit, so infer one.
-          facet.limit if facet.limit != true
-        elsif limit == -1 # limit -1 is solr-speak for unlimited
-          nil
-        else
-          limit.to_i - 1 # we added 1 to find out if we needed to paginate
-        end
-      elsif facet.limit
-        facet.limit == true ? DEFAULT_FACET_LIMIT : facet.limit
-      end
-    end
   end
 end
diff --git a/app/controllers/concerns/blacklight/search_context.rb b/app/controllers/concerns/blacklight/search_context.rb
index 5d5feb41b0..693a3e9b4b 100644
--- a/app/controllers/concerns/blacklight/search_context.rb
+++ b/app/controllers/concerns/blacklight/search_context.rb
@@ -96,7 +96,7 @@ def blacklisted_search_session_params
   def setup_next_and_previous_documents
     if search_session['counter'] && current_search_session
       index = search_session['counter'].to_i - 1
-      response, documents = get_previous_and_next_documents_for_search index, ActiveSupport::HashWithIndifferentAccess.new(current_search_session.query_params)
+      response, documents = search_service.previous_and_next_documents_for_search index, ActiveSupport::HashWithIndifferentAccess.new(current_search_session.query_params)
 
       search_session['total'] = response.total
       @search_context_response = response
diff --git a/app/controllers/concerns/blacklight/search_helper.rb b/app/controllers/concerns/blacklight/search_helper.rb
deleted file mode 100644
index 6bcac65e27..0000000000
--- a/app/controllers/concerns/blacklight/search_helper.rb
+++ /dev/null
@@ -1,155 +0,0 @@
-# frozen_string_literal: true
-# SearchHelper is a controller layer mixin. It is in the controller scope: request params, session etc.
-#
-# NOTE: Be careful when creating variables here as they may be overriding something that already exists.
-# The ActionController docs: http://api.rubyonrails.org/classes/ActionController/Base.html
-#
-# Override these methods in your own controller for customizations:
-#
-#   class CatalogController < ActionController::Base
-#     include Blacklight::Catalog
-#
-#     def repository_class
-#       MyAlternativeRepo
-#     end
-#   end
-#
-# Or by including in local extensions:
-#   module LocalSearchHelperExtension
-#     [ local overrides ]
-#   end
-#
-#   class CatalogController < ActionController::Base
-#     include Blacklight::Catalog
-#     include LocalSearchHelperExtension
-#
-#     def repository_class
-#       MyAlternativeRepo
-#     end
-#   end
-#
-# Or by using ActiveSupport::Concern:
-#
-#   module LocalSearchHelperExtension
-#     extend ActiveSupport::Concern
-#     include Blacklight::SearchHelper
-#
-#     [ local overrides ]
-#   end
-#
-#   class CatalogController < ApplicationController
-#     include LocalSearchHelperExtension
-#     include Blacklight::Catalog
-#   end
-
-module Blacklight::SearchHelper
-  extend ActiveSupport::Concern
-  include Blacklight::RequestBuilders
-
-  # a solr query method
-  # @param [Hash] user_params ({}) the user provided parameters (e.g. query, facets, sort, etc)
-  # @yield [search_builder] optional block yields configured SearchBuilder, caller can modify or create new SearchBuilder to be used. Block should return SearchBuilder to be used.
-  # @return [Blacklight::Solr::Response] the solr response object
-  def search_results(user_params)
-    builder = search_builder.with(user_params)
-    builder.page = user_params[:page] if user_params[:page]
-    builder.rows = (user_params[:per_page] || user_params[:rows]) if user_params[:per_page] || user_params[:rows]
-
-    builder = yield(builder) if block_given?
-    response = repository.search(builder)
-
-    if response.grouped? && grouped_key_for_results
-      [response.group(grouped_key_for_results), []]
-    elsif response.grouped? && response.grouped.length == 1
-      [response.grouped.first, []]
-    else
-      [response, response.documents]
-    end
-  end
-
-  # retrieve a document, given the doc id
-  # @param [Array{#to_s},#to_s] id
-  # @return [Blacklight::Solr::Response, Blacklight::SolrDocument] the solr response object and the first document
-  def fetch(id = nil, extra_controller_params = {})
-    if id.is_a? Array
-      fetch_many(id, search_state.to_h, extra_controller_params)
-    else
-      fetch_one(id, extra_controller_params)
-    end
-  end
-
-  ##
-  # Get the solr response when retrieving only a single facet field
-  # @return [Blacklight::Solr::Response] the solr response
-  def get_facet_field_response(facet_field, user_params = params || {}, extra_controller_params = {})
-    query = search_builder.with(user_params).facet(facet_field)
-    repository.search(query.merge(extra_controller_params))
-  end
-
-  # Get the previous and next document from a search result
-  # @return [Blacklight::Solr::Response, Array<Blacklight::SolrDocument>] the solr response and a list of the first and last document
-  def get_previous_and_next_documents_for_search(index, request_params, extra_controller_params = {})
-    p = previous_and_next_document_params(index)
-    query = search_builder.with(request_params).start(p.delete(:start)).rows(p.delete(:rows)).merge(extra_controller_params).merge(p)
-    response = repository.search(query)
-    document_list = response.documents
-
-    # only get the previous doc if there is one
-    prev_doc = document_list.first if index > 0
-    next_doc = document_list.last if (index + 1) < response.total
-    [response, [prev_doc, next_doc]]
-  end
-
-  # a solr query method
-  # does a standard search but returns a simplified object.
-  # an array is returned, the first item is the query string,
-  # the second item is an other array. This second array contains
-  # all of the field values for each of the documents...
-  # where the field is the "field" argument passed in.
-  def get_opensearch_response(field = nil, request_params = params || {}, extra_controller_params = {})
-    field ||= blacklight_config.view_config(:opensearch).title_field
-
-    query = search_builder.with(request_params).merge(solr_opensearch_params(field)).merge(extra_controller_params)
-    response = repository.search(query)
-
-    [response.params[:q], response.documents.flat_map { |doc| doc[field] }.uniq]
-  end
-
-  ##
-  # The key to use to retrieve the grouped field to display
-  def grouped_key_for_results
-    blacklight_config.index.group
-  end
-
-  delegate :repository_class, to: :blacklight_config
-
-  def repository
-    repository_class.new(blacklight_config)
-  end
-
-  private
-
-    ##
-    # Retrieve a set of documents by id
-    # @param [Array] ids
-    # @param [HashWithIndifferentAccess] user_params
-    # @param [HashWithIndifferentAccess] extra_controller_params
-    def fetch_many(ids, user_params, extra_controller_params)
-      user_params ||= params
-      extra_controller_params ||= {}
-
-      query = search_builder.
-                with(user_params).
-                where(blacklight_config.document_model.unique_key => ids).
-                merge(extra_controller_params).
-                merge(fl: '*')
-      solr_response = repository.search(query)
-
-      [solr_response, solr_response.documents]
-    end
-
-    def fetch_one(id, extra_controller_params)
-      solr_response = repository.find id, extra_controller_params
-      [solr_response, solr_response.documents.first]
-    end
-end
diff --git a/app/services/blacklight/search_service.rb b/app/services/blacklight/search_service.rb
new file mode 100644
index 0000000000..39a056d430
--- /dev/null
+++ b/app/services/blacklight/search_service.rb
@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+# SearchService returns search results from the repository
+module Blacklight
+  class SearchService
+    include Blacklight::RequestBuilders
+
+    def initialize(blacklight_config)
+      @blacklight_config = blacklight_config
+    end
+
+    # TODO: Can this be private?
+    attr_reader :blacklight_config
+
+    # a solr query method
+    # @param [Hash] user_params ({}) the user provided parameters (e.g. query, facets, sort, etc)
+    # @yield [search_builder] optional block yields configured SearchBuilder, caller can modify or create new SearchBuilder to be used. Block should return SearchBuilder to be used.
+    # @return [Blacklight::Solr::Response] the solr response object
+    def search_results(user_params)
+      builder = search_builder.with(user_params)
+      builder.page = user_params[:page] if user_params[:page]
+      builder.rows = (user_params[:per_page] || user_params[:rows]) if user_params[:per_page] || user_params[:rows]
+
+      builder = yield(builder) if block_given?
+      response = repository.search(builder)
+
+      if response.grouped? && grouped_key_for_results
+        [response.group(grouped_key_for_results), []]
+      elsif response.grouped? && response.grouped.length == 1
+        [response.grouped.first, []]
+      else
+        [response, response.documents]
+      end
+    end
+
+    # retrieve a document, given the doc id
+    # @param [Array{#to_s},#to_s] id
+    # @return [Blacklight::Solr::Response, Blacklight::SolrDocument] the solr response object and the first document
+    def fetch(id = nil, paging_params = {}, extra_controller_params = {})
+      if id.is_a? Array
+        fetch_many(id, paging_params, extra_controller_params)
+      else
+        fetch_one(id, extra_controller_params)
+      end
+    end
+
+    ##
+    # Get the solr response when retrieving only a single facet field
+    # @return [Blacklight::Solr::Response] the solr response
+    def facet_field_response(facet_field, user_params, extra_controller_params = {})
+      query = search_builder.with(user_params).facet(facet_field)
+      repository.search(query.merge(extra_controller_params))
+    end
+
+    # Get the previous and next document from a search result
+    # @return [Blacklight::Solr::Response, Array<Blacklight::SolrDocument>] the solr response and a list of the first and last document
+    def previous_and_next_documents_for_search(index, request_params, extra_controller_params = {})
+      p = previous_and_next_document_params(index)
+      query = search_builder.with(request_params).start(p.delete(:start)).rows(p.delete(:rows)).merge(extra_controller_params).merge(p)
+      response = repository.search(query)
+      document_list = response.documents
+
+      # only get the previous doc if there is one
+      prev_doc = document_list.first if index > 0
+      next_doc = document_list.last if (index + 1) < response.total
+      [response, [prev_doc, next_doc]]
+    end
+
+    # a solr query method
+    # does a standard search but returns a simplified object.
+    # an array is returned, the first item is the query string,
+    # the second item is an other array. This second array contains
+    # all of the field values for each of the documents...
+    # where the field is the "field" argument passed in.
+    def opensearch_response(request_params, field = nil, extra_controller_params = {})
+      field ||= blacklight_config.view_config(:opensearch).title_field
+
+      query = search_builder.with(request_params).merge(solr_opensearch_params(field)).merge(extra_controller_params)
+      response = repository.search(query)
+
+      [response.params[:q], response.documents.flat_map { |doc| doc[field] }.uniq]
+    end
+
+    ##
+    # The key to use to retrieve the grouped field to display
+    def grouped_key_for_results
+      blacklight_config.index.group
+    end
+
+    delegate :repository_class, to: :blacklight_config
+
+    def repository
+      repository_class.new(blacklight_config)
+    end
+
+    private
+
+      ##
+      # Retrieve a set of documents by id
+      # @param [Array] ids
+      # @param [HashWithIndifferentAccess] paging_params parameters about paging (e.g. :page, :per_page)
+      # @param [HashWithIndifferentAccess] extra_controller_params
+      def fetch_many(ids, paging_params, extra_controller_params)
+        extra_controller_params ||= {}
+
+        query = search_builder
+                .with(paging_params)
+                .where(blacklight_config.document_model.unique_key => ids)
+                .merge(extra_controller_params)
+                .merge(fl: '*')
+        solr_response = repository.search(query)
+
+        [solr_response, solr_response.documents]
+      end
+
+      def fetch_one(id, extra_controller_params)
+        solr_response = repository.find id, extra_controller_params
+        [solr_response, solr_response.documents.first]
+      end
+  end
+end
diff --git a/lib/blacklight/search_state.rb b/lib/blacklight/search_state.rb
index 3d37f9f2c4..2a5e9b9b51 100644
--- a/lib/blacklight/search_state.rb
+++ b/lib/blacklight/search_state.rb
@@ -31,6 +31,11 @@ def to_hash
     end
     alias to_h to_hash
 
+    # @return [Hash] parameters related to pagination
+    def page_params
+      to_h.slice(:per_page, :page)
+    end
+
     def reset
       self.class.new(ActionController::Parameters.new, blacklight_config)
     end
diff --git a/spec/controllers/catalog_controller_spec.rb b/spec/controllers/catalog_controller_spec.rb
index f66e73ea5d..9a088ae3ab 100644
--- a/spec/controllers/catalog_controller_spec.rb
+++ b/spec/controllers/catalog_controller_spec.rb
@@ -260,9 +260,11 @@
       let(:current_search) { Search.create(:query_params => { :q => ""}) }
       before do
         allow(mock_document).to receive_messages(:export_formats => {})
-        allow(controller).to receive_messages(
+        allow(controller.send(:search_service)).to receive_messages(
           fetch: [mock_response, mock_document],
-          get_previous_and_next_documents_for_search: [double(:total => 5), [double("a"), mock_document, double("b")]],
+          previous_and_next_documents_for_search: [double(:total => 5), [double("a"), mock_document, double("b")]],
+        )
+        allow(controller).to receive_messages(
           current_search_session: current_search
         )
       end
@@ -289,7 +291,7 @@
       end
 
       it "does not break if solr returns an exception" do
-        allow(controller).to receive(:get_previous_and_next_documents_for_search) {
+        allow(controller.send(:search_service)).to receive(:previous_and_next_documents_for_search) {
           raise Blacklight::Exceptions::InvalidRequest.new "Error"
         }
         get :show, params: { id: doc_id }
@@ -468,17 +470,24 @@ def export_as_mock
       end.to raise_error Blacklight::Exceptions::RecordNotFound
     end
 
-    it "redirects the user to the root url for a bad search" do
-      fake_error = Blacklight::Exceptions::InvalidRequest.new
-      allow(Rails.env).to receive_messages(:test? => false)
-      allow(controller).to receive(:search_results) { |*args| raise fake_error }
-      expect(controller.logger).to receive(:error).with(fake_error)
-      get :index, params: { q: '+' }
+    context "when there is an invalid search" do
+      let(:service) { instance_double(Blacklight::SearchService) }
+      let(:fake_error) { Blacklight::Exceptions::InvalidRequest.new }
+
+      before do
+        allow(controller).to receive(:search_service).and_return(service)
+        allow(service).to receive(:search_results) { |*args| raise fake_error }
+        allow(Rails.env).to receive_messages(:test? => false)
+      end
 
-      expect(response.redirect_url).to eq root_url
-      expect(request.flash[:notice]).to eq "Sorry, I don't understand your search."
-      expect(response).to_not be_success
-      expect(response.status).to eq 302
+      it "redirects the user to the root url for a bad search" do
+        expect(controller.logger).to receive(:error).with(fake_error)
+        get :index, params: { q: '+' }
+        expect(response.redirect_url).to eq root_url
+        expect(request.flash[:notice]).to eq "Sorry, I don't understand your search."
+        expect(response).to_not be_success
+        expect(response.status).to eq 302
+      end
     end
 
     it "returns status 500 if the catalog path is raising an exception" do
@@ -663,6 +672,58 @@ def export_as_mock
       expect(controller.send(:search_facet_path, id: "some_facet", page: 5)).to eq facet_catalog_path(id: "some_facet")
     end
   end
+
+  describe "facet_limit_for" do
+    let(:blacklight_config) { controller.blacklight_config }
+
+    it "returns specified value for facet_field specified" do
+      expect(controller.facet_limit_for("subject_topic_facet")).to eq blacklight_config.facet_fields["subject_topic_facet"].limit
+    end
+
+    it "facet_limit_hash should return hash with key being facet_field and value being configured limit" do
+      # facet_limit_hash has been removed from solrhelper in refactor. should it go back?
+      skip "facet_limit_hash has been removed from solrhelper in refactor. should it go back?"
+      expect(controller.facet_limit_hash).to eq blacklight_config[:facet][:limits]
+    end
+
+    it "handles no facet_limits in config" do
+      blacklight_config.facet_fields = {}
+      expect(controller.facet_limit_for("subject_topic_facet")).to be_nil
+    end
+
+    describe "for 'true' configured values" do
+      before do
+        allow(controller).to receive(:blacklight_config).and_return(blacklight_config)
+      end
+
+      let(:blacklight_config) do
+        Blacklight::Configuration.new do |config|
+          config.add_facet_field "language_facet", limit: true
+        end
+      end
+
+      it "returns nil if no @response available" do
+        expect(controller.facet_limit_for("some_unknown_field")).to be_nil
+      end
+
+      it "gets from @response facet.limit if available" do
+        response = instance_double(Blacklight::Solr::Response, aggregations: { "language_facet" => double(limit: nil) })
+        controller.instance_variable_set(:@response, response)
+        blacklight_config.facet_fields['language_facet'].limit = 10
+        expect(controller.facet_limit_for("language_facet")).to eq 10
+      end
+
+      it "gets the limit from the facet field in @response" do
+        response = instance_double(Blacklight::Solr::Response, aggregations: { "language_facet" => double(limit: 16) })
+        controller.instance_variable_set(:@response, response)
+        expect(controller.facet_limit_for("language_facet")).to eq 15
+      end
+
+      it "defaults to 10" do
+        expect(controller.facet_limit_for("language_facet")).to eq 10
+      end
+    end
+  end
 end
 
 # there must be at least one facet, and each facet must have at least one value
diff --git a/spec/controllers/blacklight/search_helper_spec.rb b/spec/services/blacklight/search_service_spec.rb
similarity index 66%
rename from spec/controllers/blacklight/search_helper_spec.rb
rename to spec/services/blacklight/search_service_spec.rb
index 2bc7553ef9..5af77598c0 100644
--- a/spec/controllers/blacklight/search_helper_spec.rb
+++ b/spec/services/blacklight/search_service_spec.rb
@@ -8,44 +8,22 @@
 #  to talk with solr and get results)? when we do a document request, does
 #  blacklight code get a single document returned?)
 #
-describe Blacklight::SearchHelper do
-
-  # SearchHelper is a controller layer mixin, which depends
-  # on being mixed into a class which has #params (from Rails)
-  # and #blacklight_config
-  class SearchHelperTestClass
-    include Blacklight::SearchHelper
-
-    attr_accessor :blacklight_config
-    attr_accessor :repository
-
-    def initialize blacklight_config, conn
-      self.blacklight_config = blacklight_config
-      self.repository = Blacklight::Solr::Repository.new(blacklight_config)
-      self.repository.connection = conn
-    end
-
-    def params
-      {}
-    end
-  end
-
-  subject { SearchHelperTestClass.new blacklight_config, blacklight_solr }
+describe Blacklight::SearchService do
 
+  let(:service) { described_class.new(blacklight_config) }
+  subject { service }
   let(:blacklight_config) { Blacklight::Configuration.new }
   let(:copy_of_catalog_config) { ::CatalogController.blacklight_config.deep_copy }
   let(:blacklight_solr) { RSolr.connect(Blacklight.connection_config.except(:adapter)) }
 
-  before(:each) do
-    @all_docs_query = ''
-    @no_docs_query = 'zzzzzzzzzzzz'
-    @single_word_query = 'include'
-    @mult_word_query = 'tibetan history'
+  let(:all_docs_query) { '' }
+  let(:no_docs_query) { 'zzzzzzzzzzzz' }
   #  f[format][]=Book&f[language_facet][]=English
-    @single_facet = {:format=>'Book'}
-    @multi_facets = {:format=>'Book', :language_facet=>'Tibetan'}
-    @bad_facet = {:format=>'666'}
-    @subject_search_params = {:commit=>"search", :search_field=>"subject", :action=>"index", :"controller"=>"catalog", :"rows"=>"10", :"q"=>"wome"}
+  let(:single_facet) { { format: 'Book' } }
+
+  before do
+    allow(service).to receive(:repository).and_return(Blacklight::Solr::Repository.new(blacklight_config))
+    service.repository.connection = blacklight_solr
   end
 
   # SPECS FOR SEARCH RESULTS FOR QUERY
@@ -55,7 +33,7 @@ def params
     describe 'for a sample query returning results' do
 
       before do
-        (@solr_response, @document_list) = subject.search_results(q: @all_docs_query)
+        (@solr_response, @document_list) = service.search_results(q: all_docs_query)
       end
 
       it "uses the configured request handler" do
@@ -66,7 +44,7 @@ def params
           expect(params[:params]["facet.query"]).to eq ["pub_date:[#{5.years.ago.year} TO *]", "pub_date:[#{10.years.ago.year} TO *]", "pub_date:[#{25.years.ago.year} TO *]"]
           expect(params[:params]).to include('rows' => 10, 'qt'=>"custom_request_handler", 'q'=>"", "f.subject_topic_facet.facet.limit"=>21, 'sort'=>"score desc, pub_date_sort desc, title_sort asc")
         end.and_return({'response'=>{'docs'=>[]}})
-        subject.search_results(q: @all_docs_query)
+        service.search_results(q: all_docs_query)
       end
 
       it 'has a @response.docs list of the same size as @document_list' do
@@ -92,7 +70,7 @@ def params
       before do
         blacklight_config.default_solr_params[:group] = true
         blacklight_config.default_solr_params[:'group.field'] = 'pub_date_sort'
-        (@solr_response, @document_list) = subject.search_results(q: @all_docs_query)
+        (@solr_response, @document_list) = service.search_results(q: all_docs_query)
       end
 
       it "has an empty document list" do
@@ -112,7 +90,7 @@ def params
         allow(subject).to receive_messages grouped_key_for_results: 'title_sort'
         blacklight_config.default_solr_params[:group] = true
         blacklight_config.default_solr_params[:'group.field'] = ['pub_date_sort', 'title_sort']
-        (@solr_response, @document_list) = subject.search_results(q: @all_docs_query)
+        (@solr_response, @document_list) = service.search_results(q: all_docs_query)
       end
 
       it "has an empty document list" do
@@ -128,7 +106,7 @@ def params
 
     describe "for All Docs Query and One Facet" do
       it 'has results' do
-        (solr_response, document_list) = subject.search_results(q: @all_docs_query, f: @single_facet)
+        (solr_response, document_list) = service.search_results(q: all_docs_query, f: single_facet)
         expect(solr_response.docs).to have(document_list.size).results
         expect(solr_response.docs).to have_at_least(1).result
       end
@@ -138,7 +116,7 @@ def params
 
     describe "for Query Without Results and No Facet" do
       it 'has no results and not raise error' do
-        (solr_response, document_list) = subject.search_results(q: @no_docs_query)
+        (solr_response, document_list) = service.search_results(q: no_docs_query)
         expect(document_list).to have(0).results
         expect(solr_response.docs).to have(0).results
       end
@@ -146,15 +124,17 @@ def params
 
     describe "for Query Without Results and One Facet" do
       it 'has no results and not raise error' do
-        (solr_response, document_list) = subject.search_results(q: @no_docs_query, f: @single_facet)
+        (solr_response, document_list) = service.search_results(q: no_docs_query, f: single_facet)
         expect(document_list).to have(0).results
         expect(solr_response.docs).to have(0).results
       end
     end
 
     describe "for All Docs Query and Bad Facet" do
+      let(:bad_facet) { { format: '666' } }
+
       it 'has no results and not raise error' do
-        (solr_response, document_list) = subject.search_results(q: @all_docs_query, f: @bad_facet)
+        (solr_response, document_list) = service.search_results(q: all_docs_query, f: bad_facet)
         expect(document_list).to have(0).results
         expect(solr_response.docs).to have(0).results
       end
@@ -168,7 +148,7 @@ def params
     let(:blacklight_config) { copy_of_catalog_config }
 
     before do
-      (solr_response, document_list) = subject.search_results(q: @all_docs_query)
+      (solr_response, document_list) = service.search_results(q: all_docs_query)
       @facets = solr_response.aggregations
     end
 
@@ -210,44 +190,44 @@ def params
     let(:blacklight_config) { copy_of_catalog_config }
 
     it 'starts with first results by default' do
-      (solr_response, document_list) = subject.search_results(q: @all_docs_query)
+      (solr_response, document_list) = service.search_results(q: all_docs_query)
       expect(solr_response.params[:start].to_i).to eq 0
     end
     it 'has number of results (per page) set in initializer, by default' do
-      (solr_response, document_list) = subject.search_results(q: @all_docs_query)
+      (solr_response, document_list) = service.search_results(q: all_docs_query)
       expect(solr_response.docs).to have(blacklight_config[:default_solr_params][:rows]).items
       expect(document_list).to have(blacklight_config[:default_solr_params][:rows]).items
     end
 
     it 'gets number of results per page requested' do
       num_results = 3  # non-default value
-      (solr_response1, document_list1) = subject.search_results(q: @all_docs_query, per_page: num_results)
+      (solr_response1, document_list1) = service.search_results(q: all_docs_query, per_page: num_results)
       expect(document_list1).to have(num_results).docs
       expect(solr_response1.docs).to have(num_results).docs
     end
 
     it 'gets number of rows requested' do
       num_results = 4  # non-default value
-      (solr_response1, document_list1) = subject.search_results(q: @all_docs_query, rows: num_results)
+      (solr_response1, document_list1) = service.search_results(q: all_docs_query, rows: num_results)
       expect(document_list1).to have(num_results).docs
       expect(solr_response1.docs).to have(num_results).docs
     end
 
     it 'skips appropriate number of results when requested - default per page' do
       page = 3
-      (solr_response2, document_list2) = subject.search_results(q: @all_docs_query, page: page)
+      (solr_response2, document_list2) = service.search_results(q: all_docs_query, page: page)
       expect(solr_response2.params[:start].to_i).to eq  blacklight_config[:default_solr_params][:rows] * (page-1)
     end
     it 'skips appropriate number of results when requested - non-default per page' do
       page = 3
       num_results = 3
-      (solr_response2a, document_list2a) = subject.search_results(q: @all_docs_query, per_page: num_results, page: page)
+      (solr_response2a, document_list2a) = service.search_results(q: all_docs_query, per_page: num_results, page: page)
       expect(solr_response2a.params[:start].to_i).to eq num_results * (page-1)
     end
 
     it 'has no results when prompted for page after last result' do
       big = 5000
-      (solr_response3, document_list3) = subject.search_results(q: @all_docs_query, rows: big, page: big)
+      (solr_response3, document_list3) = service.search_results(q: all_docs_query, rows: big, page: big)
       expect(document_list3).to have(0).docs
       expect(solr_response3.docs).to have(0).docs
     end
@@ -255,12 +235,12 @@ def params
     it 'shows first results when prompted for page before first result' do
       # FIXME: should it show first results, or should it throw an error for view to deal w?
       #   Solr throws an error for a negative start value
-      (solr_response4, document_list4) = subject.search_results(q: @all_docs_query, page: '-1')
+      (solr_response4, document_list4) = service.search_results(q: all_docs_query, page: '-1')
       expect(solr_response4.params[:start].to_i).to eq 0
     end
     it 'has results available when asked for more than are in response' do
       big = 5000
-      (solr_response5, document_list5) = subject.search_results(q: @all_docs_query, rows: big, page: 1)
+      (solr_response5, document_list5) = service.search_results(q: all_docs_query, rows: big, page: 1)
       expect(solr_response5.docs).to have(document_list5.length).docs
       expect(solr_response5.docs).to have_at_least(1).doc
     end
@@ -269,22 +249,23 @@ def params
 
   # SPECS FOR SINGLE DOCUMENT REQUESTS
   describe 'Get Document By Id', :integration => true do
+    let(:doc_id) { '2007020969' }
+    let(:bad_id) { 'redrum' }
+
     before do
-      @doc_id = '2007020969'
-      @bad_id = "redrum"
-      @response2, @document = subject.fetch(@doc_id)
+      @response2, @document = service.fetch(doc_id)
     end
 
     it "raises Blacklight::RecordNotFound for an unknown id" do
       expect {
-        subject.fetch(@bad_id)
+        service.fetch(bad_id)
       }.to raise_error(Blacklight::Exceptions::RecordNotFound)
     end
 
     it "uses a provided document solr path" do
-      allow(blacklight_config).to receive_messages(:document_solr_path => 'get')
+      allow(blacklight_config).to receive_messages(document_solr_path: 'get')
       allow(blacklight_solr).to receive(:send_and_receive).with('get', kind_of(Hash)).and_return({'response'=>{'docs'=>[]}})
-      expect { subject.fetch(@doc_id)}.to raise_error Blacklight::Exceptions::RecordNotFound
+      expect { service.fetch(doc_id)}.to raise_error Blacklight::Exceptions::RecordNotFound
     end
 
     it "has a non-nil result for a known id" do
@@ -294,7 +275,7 @@ def params
       expect(@response2.docs.size).to eq 1
     end
     it 'has the expected value in the id field' do
-      expect(@document.id).to eq @doc_id
+      expect(@document.id).to eq doc_id
     end
     it 'has non-nil values for required fields set in initializer' do
       expect(@document.fetch(blacklight_config.view_config(:show).display_type_field)).not_to be_nil
@@ -304,13 +285,13 @@ def params
 # SPECS FOR SPELLING SUGGESTIONS VIA SEARCH
   describe "Searches should return spelling suggestions", :integration => true do
     it 'search results for just-poor-enough-query term should have (multiple) spelling suggestions' do
-      (solr_response, document_list) = subject.search_results(q: 'boo')
+      (solr_response, document_list) = service.search_results(q: 'boo')
       expect(solr_response.spelling.words).to include('bon')
       expect(solr_response.spelling.words).to include('bod')  #for multiple suggestions
     end
 
     it 'search results for just-poor-enough-query term should have multiple spelling suggestions' do
-      (solr_response, document_list) = subject.search_results(q: 'politica')
+      (solr_response, document_list) = service.search_results(q: 'politica')
       expect(solr_response.spelling.words).to include('policy') # less freq
       expect(solr_response.spelling.words).to include('politics') # more freq
       expect(solr_response.spelling.words).to include('political') # more freq
@@ -323,17 +304,17 @@ def params
     end
 
     it "title search results for just-poor-enough query term should have spelling suggestions" do
-      (solr_response, document_list) = subject.search_results(q: 'yehudiyam', qt: 'search', :"spellcheck.dictionary" => "title")
+      (solr_response, document_list) = service.search_results(q: 'yehudiyam', qt: 'search', :"spellcheck.dictionary" => "title")
       expect(solr_response.spelling.words).to include('yehudiyim')
     end
 
     it "author search results for just-poor-enough-query term should have spelling suggestions" do
-      (solr_response, document_list) = subject.search_results(q: 'shirma', qt: 'search', :"spellcheck.dictionary" => "author")
+      (solr_response, document_list) = service.search_results(q: 'shirma', qt: 'search', :"spellcheck.dictionary" => "author")
       expect(solr_response.spelling.words).to include('sharma')
     end
 
     it "subject search results for just-poor-enough-query term should have spelling suggestions" do
-      (solr_response, document_list) = subject.search_results(q: 'wome', qt: 'search', :"spellcheck.dictionary" => "subject")
+      (solr_response, document_list) = service.search_results(q: 'wome', qt: 'search', :"spellcheck.dictionary" => "subject")
       expect(solr_response.spelling.words).to include('women')
     end
 
@@ -344,49 +325,6 @@ def params
 
   end
 
-  describe "facet_limit_for" do
-    let(:blacklight_config) { copy_of_catalog_config }
-
-    it "returns specified value for facet_field specified" do
-      expect(subject.facet_limit_for("subject_topic_facet")).to eq blacklight_config.facet_fields["subject_topic_facet"].limit
-    end
-
-    it "facet_limit_hash should return hash with key being facet_field and value being configured limit" do
-      # facet_limit_hash has been removed from solrhelper in refactor. should it go back?
-      skip "facet_limit_hash has been removed from solrhelper in refactor. should it go back?"
-      expect(subject.facet_limit_hash).to eq blacklight_config[:facet][:limits]
-    end
-
-    it "handles no facet_limits in config" do
-      blacklight_config.facet_fields = {}
-      expect(subject.facet_limit_for("subject_topic_facet")).to be_nil
-    end
-
-    describe "for 'true' configured values" do
-      let(:blacklight_config) do
-        Blacklight::Configuration.new do |config|
-          config.add_facet_field "language_facet", limit: true
-        end
-      end
-      it "returns nil if no @response available" do
-        expect(subject.facet_limit_for("some_unknown_field")).to be_nil
-      end
-      it "gets from @response facet.limit if available" do        
-        @response = instance_double(Blacklight::Solr::Response, aggregations: { "language_facet" => double(limit: nil) })
-        subject.instance_variable_set(:@response, @response)
-        blacklight_config.facet_fields['language_facet'].limit = 10
-        expect(subject.facet_limit_for("language_facet")).to eq 10
-      end
-      it "gets the limit from the facet field in @response" do
-        @response = instance_double(Blacklight::Solr::Response, aggregations: { "language_facet" => double(limit: 16) })
-        subject.instance_variable_set(:@response, @response)
-        expect(subject.facet_limit_for("language_facet")).to eq 15
-      end
-      it "defaults to 10" do
-        expect(subject.facet_limit_for("language_facet")).to eq 10
-      end
-    end
-  end
 
 # TODO:  more complex queries!  phrases, offset into search results, non-latin, boosting(?)
 #  search within query building (?)
@@ -397,44 +335,43 @@ def params
 #  nearby on shelf
   it "raises a Blacklight exception if RSolr can't connect to the Solr instance" do
     allow(blacklight_solr).to receive(:send_and_receive).and_raise(Errno::ECONNREFUSED)
-    expect { subject.repository.search }.to raise_exception(/Unable to connect to Solr instance/)
+    expect { service.repository.search }.to raise_exception(/Unable to connect to Solr instance/)
   end
 
   describe "grouped_key_for_results" do
     it "pulls the grouped key out of the config" do
       blacklight_config.index.group = 'xyz'
-      expect(subject.grouped_key_for_results).to eq('xyz')
-    end 
+      expect(service.grouped_key_for_results).to eq('xyz')
+    end
   end
 
-  describe "#get_previous_and_next_documents_for_search" do
-    let(:pre_query) { SearchHelperTestClass.new blacklight_config, blacklight_solr }
+  describe "#previous_and_next_documents_for_search" do
     before do
-      @full_response, @all_docs = pre_query.search_results(q: '', per_page: '100')
+      @full_response, @all_docs = service.search_results(q: '', per_page: '100')
     end
 
     it "returns the previous and next documents for a search" do
-      response, docs = subject.get_previous_and_next_documents_for_search(4, :q => '')
+      response, docs = service.previous_and_next_documents_for_search(4, :q => '')
 
       expect(docs.first.id).to eq @all_docs[3].id
       expect(docs.last.id).to eq @all_docs[5].id
     end
 
     it "returns only the next document if the counter is 0" do
-      response, docs = subject.get_previous_and_next_documents_for_search(0, :q => '')
+      response, docs = service.previous_and_next_documents_for_search(0, :q => '')
 
       expect(docs.first).to be_nil
       expect(docs.last.id).to eq @all_docs[1].id
     end
 
     it "returns only the previous document if the counter is the total number of documents" do
-      response, docs = subject.get_previous_and_next_documents_for_search(@full_response.total - 1, :q => '')
+      response, docs = service.previous_and_next_documents_for_search(@full_response.total - 1, :q => '')
       expect(docs.first.id).to eq @all_docs.slice(-2).id
       expect(docs.last).to be_nil
     end
 
     it "returns an array of nil values if there is only one result" do
-      response, docs = subject.get_previous_and_next_documents_for_search(0, :q => 'id:2007020969')
+      response, docs = service.previous_and_next_documents_for_search(0, :q => 'id:2007020969')
       expect(docs.last).to be_nil
       expect(docs.first).to be_nil
     end
diff --git a/spec/views/catalog/_paginate_compact.html.erb_spec.rb b/spec/views/catalog/_paginate_compact.html.erb_spec.rb
index 2907ff732f..ba4439f3cb 100644
--- a/spec/views/catalog/_paginate_compact.html.erb_spec.rb
+++ b/spec/views/catalog/_paginate_compact.html.erb_spec.rb
@@ -1,35 +1,12 @@
 # frozen_string_literal: true
 
 describe "catalog/_paginate_compact.html.erb" do
-  let(:user) { User.new.tap { |u| u.save(validate: false) } }
+  let(:user) { User.new { |u| u.save(validate: false) } }
 
   before do
     controller.request.path_parameters[:action] = 'index'
   end
 
-  describe "with a real solr response", :integration => true do
-    def blacklight_config
-      @config ||= CatalogController.blacklight_config
-    end
-
-    def blacklight_config=(config)
-      @config = config
-    end
-
-    def facet_limit_for *args
-      0
-    end
-
-    include Blacklight::SearchHelper
-
-    it "renders solr responses" do
-      solr_response, document_list = search_results(q: '')
-      render :partial => 'catalog/paginate_compact', :object => solr_response
-      expect(rendered).to have_selector ".page-entries"
-      expect(rendered).to have_selector "a[@rel=next]"
-    end
-  end
-
   it "renders paginatable arrays" do
     render :partial => 'catalog/paginate_compact', :object => (Kaminari.paginate_array([], total_count: 145).page(1).per(10))
     expect(rendered).to have_selector ".page-entries"