Consistently use #search_action_url (and #search_action_path) to prov…

…ide Blacklight urls for facets, links to searches, etc

app/controllers/bookmarks_controller.rb

@@ -15,7 +15,6 @@ class BookmarksController < CatalogController
   def search_action_url *args
     catalog_index_url *args
   end
-  helper_method :search_action_url
 
   before_filter :verify_user
 

app/helpers/blacklight/blacklight_helper_behavior.rb

@@ -364,7 +364,7 @@ def get_field_values document, field, field_config, options = {}
         end
 
         Array(value).map do |v|
-          link_to render_field_value(v, field_config), search_action_url(add_facet_params(link_field, v, {}))
+          link_to render_field_value(v, field_config), search_action_path(add_facet_params(link_field, v, {}))
         end if field
       else
         value

app/helpers/blacklight/facets_helper_behavior.rb

@@ -113,9 +113,8 @@ def facet_partial_name(display_facet = nil)
   # @option options [Boolean] :suppress_link display the facet, but don't link to it
   # @option options [Rails::Engine] :route_set route set to use to render the link
   # @return [String]
-  def render_facet_value(facet_solr_field, item, options ={})    
-    scope = options.delete(:route_set) || self
-    path = scope.url_for(add_facet_params_and_redirect(facet_solr_field, item).merge(only_path: true))
+  def render_facet_value(facet_solr_field, item, options ={})
+    path = search_action_path(add_facet_params_and_redirect(facet_solr_field, item))
     content_tag(:span, :class => "facet-label") do
       link_to_unless(options[:suppress_link], facet_display_value(facet_solr_field, item), path, :class=>"facet_select")
     end + render_facet_count(item.hits)
@@ -128,7 +127,7 @@ def render_selected_facet_value(facet_solr_field, item)
     content_tag(:span, :class => "facet-label") do
       content_tag(:span, facet_display_value(facet_solr_field, item), :class => "selected") +
       # remove link
-      link_to(content_tag(:span, '', :class => "glyphicon glyphicon-remove") + content_tag(:span, '[remove]', :class => 'sr-only'), remove_facet_params(facet_solr_field, item, params), :class=>"remove")
+      link_to(content_tag(:span, '', :class => "glyphicon glyphicon-remove") + content_tag(:span, '[remove]', :class => 'sr-only'), search_action_path(remove_facet_params(facet_solr_field, item, params)), :class=>"remove")
     end + render_facet_count(item.hits, :classes => ["selected"])
   end
 

app/helpers/blacklight/render_constraints_helper_behavior.rb

@@ -78,7 +78,7 @@ def render_filter_element(facet, values, localized_params)
     safe_join(values.map do |val|
       render_constraint_element( blacklight_config.facet_fields[facet].label,
                   facet_display_value(facet, val),
-                  :remove => url_for(remove_facet_params(facet, val, localized_params)),
+                  :remove => search_action_path(remove_facet_params(facet, val, localized_params)),
                   :classes => ["filter", "filter-" + facet.parameterize]
                 )
     end, "\n")

app/helpers/blacklight/url_helper_behavior.rb

@@ -1,27 +1,35 @@
 module Blacklight::UrlHelperBehavior
 
+  ##
+  # Extension point for downstream applications
+  # to provide more interesting routing to 
+  # documents
+  def url_for_document doc
+    doc
+  end
+
   # link_to_document(doc, :label=>'VIEW', :counter => 3)
   # Use the catalog_path RESTful route to create a link to the show page for a specific item.
   # catalog_path accepts a HashWithIndifferentAccess object. The solr query params are stored in the session,
   # so we only need the +counter+ param here. We also need to know if we are viewing to document as part of search results.
   def link_to_document(doc, opts={:label=>nil, :counter => nil})
     opts[:label] ||= document_show_link_field(doc)
     label = render_document_index_label doc, opts
-    link_to label, doc, search_session_params(opts[:counter]).merge(opts.reject { |k,v| [:label, :counter].include? k  })
+    link_to label, url_for_document(doc), search_session_params(opts[:counter]).merge(opts.reject { |k,v| [:label, :counter].include? k  })
   end
 
   ##
   # Link to the previous document in the current search context
   def link_to_previous_document(previous_document)
-    link_to_unless previous_document.nil?, raw(t('views.pagination.previous')), previous_document, search_session_params(search_session[:counter].to_i - 1).merge(:class => "previous", :rel => 'prev')  do
+    link_to_unless previous_document.nil?, raw(t('views.pagination.previous')), url_for_document(previous_document), search_session_params(search_session[:counter].to_i - 1).merge(:class => "previous", :rel => 'prev')  do
       content_tag :span, raw(t('views.pagination.previous')), :class => 'previous'
     end
   end
 
   ##
   # Link to the next document in the current search context
   def link_to_next_document(next_document)
-    link_to_unless next_document.nil?, raw(t('views.pagination.next')), next_document, search_session_params(search_session[:counter].to_i + 1).merge(:class => "next", :rel => 'next') do
+    link_to_unless next_document.nil?, raw(t('views.pagination.next')), url_for_document(next_document), search_session_params(search_session[:counter].to_i + 1).merge(:class => "next", :rel => 'next') do
       content_tag :span, raw(t('views.pagination.next')), :class => 'next'
     end
   end
@@ -40,7 +48,7 @@ def search_session_params counter
   def link_to_query(query)
     p = params.except(:page, :action)
     p[:q]=query
-    link_url = catalog_index_path(p)
+    link_url = search_action_path(p)
     link_to(query, link_url)
   end
 
@@ -52,7 +60,7 @@ def start_over_path query_params = params
     current_index_view_type = document_index_view_type(query_params)
     h[:view] = current_index_view_type unless current_index_view_type == default_document_index_view_type
 
-    search_action_url(h)
+    search_action_path(h)
   end
 
   # Create a link back to the index screen, keeping the user's facet, query and paging choices intact by using session.
@@ -76,7 +84,7 @@ def link_back_to_catalog(opts={:label=>nil})
 
   # Search History and Saved Searches display
   def link_to_previous_search(params)
-    link_to(render_search_to_s(params), catalog_index_path(params))
+    link_to(render_search_to_s(params), search_action_path(params))
   end
 
   # @overload params_for_search(source_params, params_to_merge)
@@ -126,7 +134,13 @@ def sanitize_search_params source_params
     my_params = source_params.reject { |k,v| v.nil? }
 
     my_params.except(:action, :controller, :id, :commit, :utf8)
+  end
 
+  ##
+  # Reset any search parameters that store search context
+  # and need to be reset when e.g. constraints change
+  def reset_search_params source_params
+    sanitize_search_params(source_params).except :page, :counter
   end
 
   # adds the value and/or field to params[:f]
@@ -143,7 +157,7 @@ def add_facet_params(field, item, source_params = params)
 
     value = facet_value_for_facet_item(item)
 
-    p = source_params.dup
+    p = reset_search_params(source_params)
     p[:f] = (p[:f] || {}).dup # the command above is not deep in rails3, !@#$!@#$
     p[:f][field] = (p[:f][field] || []).dup
 
@@ -171,15 +185,12 @@ def add_facet_params(field, item, source_params = params)
   # catalog/index with their new facet choice. 
   def add_facet_params_and_redirect(field, item)
     new_params = add_facet_params(field, item)
-    new_params.except! :page, :id
 
     # Delete any request params from facet-specific action, needed
     # to redir to index action properly. 
     new_params.except! *Blacklight::Solr::FacetPaginator.request_keys.values
 
-    # Force action to be index. 
-    new_params[:action] = "index"
-    new_params    
+    new_params 
   end
 
   # copies the current params (or whatever is passed in as the 3rd arg)
@@ -193,13 +204,12 @@ def remove_facet_params(field, item, source_params=params)
 
     value = facet_value_for_facet_item(item)
 
-    p = source_params.dup
+    p = reset_search_params(source_params)
     # need to dup the facet values too,
     # if the values aren't dup'd, then the values
     # from the session will get remove in the show view...
     p[:f] = (p[:f] || {}).dup
     p[:f][field] = (p[:f][field] || []).dup
-    p.except! :page, :id, :counter, :commit
     p[:f][field] = p[:f][field] - [value]
     p[:f].delete(field) if p[:f][field].size == 0
     p

lib/blacklight/catalog.rb

@@ -192,7 +192,7 @@ def render_facet_list_as_json
     # By default, any search action from a Blacklight::Catalog controller
     # should use the current controller when constructing the route.
     def search_action_url options = {}
-      url_for(options.merge(:action => 'index', :only_path => true))
+      url_for(options.merge(:action => 'index'))
     end
 
     # extract the pagination info from the response object

lib/blacklight/controller.rb

@@ -23,7 +23,7 @@ module Blacklight::Controller
     # extra head content
     helper_method :has_user_authentication_provider?
     helper_method :blacklight_config
-    helper_method :search_action_url
+    helper_method :search_action_url, :search_action_path
 
 
     # This callback runs when a user first logs in
@@ -50,6 +50,15 @@ def search_action_url *args
       catalog_index_url *args
     end
 
+    def search_action_path *args
+
+      if args.first.is_a? Hash
+        args.first[:only_path] = true
+      end
+
+      search_action_url *args
+    end
+
     # Returns a list of Searches from the ids in the user's history.
     def searches_from_history
       session[:history].blank? ? Search.none : Search.where(:id => session[:history]).order("updated_at desc")

spec/features/alternate_controller_spec.rb

@@ -3,7 +3,7 @@
 describe "Alternate Controller Behaviors" do
   it "should have the correct per-page form" do
     visit alternate_index_path
-    page.should have_selector("form[action='#{alternate_index_path}']")
+    page.should have_selector("form[action='#{alternate_index_url}']")
     fill_in "q", :with=>"history"
     click_button 'search'
     expect(current_path).to match /#{alternate_index_path}/
@@ -15,7 +15,7 @@
 
   it "should have the correct search field form" do
     visit alternate_index_path
-    page.should have_selector("form[action='#{alternate_index_path}']")
+    page.should have_selector("form[action='#{alternate_index_url}']")
     fill_in "q", :with=>"history"
     click_button 'search'
     expect(current_path).to match /#{alternate_index_path}/
@@ -25,7 +25,7 @@
 
   it "should display document thumbnails" do
     visit alternate_index_path
-    page.should have_selector("form[action='#{alternate_index_path}']")
+    page.should have_selector("form[action='#{alternate_index_url}']")
     fill_in "q", :with=>"history"
     click_button 'search'
     expect(page).to have_selector ".document-thumbnail"

spec/helpers/blacklight_helper_spec.rb

@@ -12,7 +12,7 @@ def blacklight_config
   end
 
   before(:each) do
-    helper.stub(:search_action_url) do |*args|
+    helper.stub(:search_action_path) do |*args|
       catalog_index_url *args
     end
   end
@@ -155,14 +155,14 @@ def mock_document_app_helper_url *args
       doc = double()
       doc.should_receive(:get).with('link_to_search_true', :sep => nil).and_return('x')
       value = helper.render_index_field_value :document => doc, :field => 'link_to_search_true'
-      expect(value).to eq helper.link_to("x", helper.search_action_url(:f => { :link_to_search_true => ['x'] }))
+      expect(value).to eq helper.link_to("x", helper.search_action_path(:f => { :link_to_search_true => ['x'] }))
     end
 
     it "should check for a link_to_search with a field name" do
       doc = double()
       doc.should_receive(:get).with('link_to_search_named', :sep => nil).and_return('x')
       value = helper.render_index_field_value :document => doc, :field => 'link_to_search_named'
-      expect(value).to eq helper.link_to("x", helper.search_action_url(:f => { :some_field => ['x'] }))
+      expect(value).to eq helper.link_to("x", helper.search_action_path(:f => { :some_field => ['x'] }))
     end
 
     it "should gracefully handle when no highlight field is available" do
@@ -253,14 +253,14 @@ def mock_document_app_helper_url *args
       doc = double()
       doc.should_receive(:get).with('link_to_search_true', :sep => nil).and_return('x')
       value = helper.render_document_show_field_value :document => doc, :field => 'link_to_search_true'
-      expect(value).to eq helper.link_to("x", helper.search_action_url(:f => { :link_to_search_true => ['x'] }))
+      expect(value).to eq helper.link_to("x", helper.search_action_path(:f => { :link_to_search_true => ['x'] }))
     end
 
     it "should check for a link_to_search with a field name" do
       doc = double()
       doc.should_receive(:get).with('link_to_search_named', :sep => nil).and_return('x')
       value = helper.render_document_show_field_value :document => doc, :field => 'link_to_search_named'
-      expect(value).to eq helper.link_to("x", helper.search_action_url(:f => { :some_field => ['x'] }))
+      expect(value).to eq helper.link_to("x", helper.search_action_path(:f => { :some_field => ['x'] }))
     end
 
     it "should gracefully handle when no highlight field is available" do

spec/helpers/facets_helper_spec.rb

@@ -299,9 +299,13 @@
       helper.stub(:facet_configuration_for_field).with('simple_field').and_return(double(:query => nil, :date => nil, :helper_method => nil, :single => false))
       helper.should_receive(:facet_display_value).and_return('Z')
       helper.should_receive(:add_facet_params_and_redirect).and_return({controller:'catalog'})
+
+      helper.stub(:search_action_path) do |*args|
+        catalog_index_path *args
+      end
     end
     describe "simple case" do
-      let(:expected_html) { "<span class=\"facet-label\"><a class=\"facet_select\" href=\"/\">Z</a></span><span class=\"facet-count\">10</span>" }
+      let(:expected_html) { "<span class=\"facet-label\"><a class=\"facet_select\" href=\"/catalog\">Z</a></span><span class=\"facet-count\">10</span>" }
       it "should use facet_display_value" do
         result = helper.render_facet_value('simple_field', item)
         expect(result).to be_equivalent_to(expected_html).respecting_element_order
@@ -315,17 +319,6 @@
         expect(result).to be_equivalent_to(expected_html).respecting_element_order
       end
     end
-
-    describe "when a route_set is passed" do
-      let(:my_engine) { double("Engine") }
-      let(:expected_html) { "<span class=\"facet-label\"><a class=\"facet_select\" href=\"/\">Z</a></span><span class=\"facet-count\">10</span>" }
-
-      it "should use the engine scope" do
-        expect(my_engine).to receive(:url_for).and_return({controller: 'catalog'})
-        result = helper.render_facet_value('simple_field', item, route_set: my_engine)
-        expect(result).to be_equivalent_to(expected_html).respecting_element_order
-      end
-    end
   end
 
   describe "#facet_display_value" do

spec/helpers/render_constraints_helper_spec.rb

@@ -5,6 +5,9 @@
   before do
     # the helper methods below infer paths from the current route
     controller.request.path_parameters["controller"] = 'catalog'
+    helper.stub(:search_action_path) do |*args|
+      catalog_index_path *args
+    end
   end
 
   describe '#render_constraints_query' do
@@ -23,7 +26,7 @@
     it "should have a link relative to the current url" do
       result = helper.render_filter_element('type', ['journal'], {:q=>'biz'})
       # I'm not certain how the ampersand gets in there. It's not important.
-      expect(result).to have_selector "a[href='/?&q=biz']"
+      expect(result).to have_link "Remove constraint Type: journal", href: "/catalog?&q=biz"
     end
   end