Refactor the signature of link_to_document

The second argument should now be a symbol of the field, a proc to
evaluate or a string. The third arugment is the options hash.

app/helpers/blacklight/blacklight_helper_behavior.rb

@@ -9,6 +9,7 @@ module Blacklight::BlacklightHelperBehavior
   include HashAsHiddenFieldsHelper
   include RenderConstraintsHelper
   include FacetsHelper
+  extend Deprecation
 
   ##
   # Get the name of this application, from either:
@@ -514,7 +515,7 @@ def render_document_partial(doc, base_name, locals = {})
   # The partial names will be interpolated with the following variables:
   #   - action_name: (e.g. index, show)
   #   - index_view_type: (the current view type, e.g. list, gallery)
-  #   - format: the document's format (e.g. book) 
+  #   - format: the document's format (e.g. book)
   #
   # @see #render_document_partial
   def document_partial_path_templates
@@ -528,12 +529,18 @@ def document_partial_path_templates
   # Render the document index heading
   #
   # @param [SolrDocument] doc
-  # @param [Hash] opts
+  # @param [Hash] opts (deprecated)
   # @option opts [Symbol] :label Render the given field from the document
   # @option opts [Proc] :label Evaluate the given proc
   # @option opts [String] :label Render the given string
-  def render_document_index_label doc, opts = {}
-    presenter(doc).render_document_index_label opts
+  # @param [Symbol, Proc, String] field Render the given field or evaluate the proc or render the given string
+  def render_document_index_label doc, field, opts = {}
+    Deprecation.warn self, "render_document_index_label is deprecated"
+    if field.kind_of? Hash
+      Deprecation.warn self, "Calling render_document_index_label with a hash is deprecated"
+      field = field[:label]
+    end
+    presenter(doc).render_document_index_label field, opts
   end
 
   ##

app/helpers/blacklight/catalog_helper_behavior.rb

@@ -176,7 +176,7 @@ def render_thumbnail_tag document, image_options = {}, url_options = {}
       if url_options === false || url_options[:suppress_link]
         value
       else
-        link_to_document document, url_options.merge(:label => value)
+        link_to_document document, value, url_options
       end
     end
   end

app/helpers/blacklight/url_helper_behavior.rb

@@ -19,13 +19,24 @@ def url_for_document doc, options = {}
     end
   end
 
+  # link_to_document(doc, 'VIEW', :counter => 3)
   # 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
+  def link_to_document(doc, field_or_opts = nil, opts={:counter => nil})
+    if field_or_opts.kind_of? Hash
+      opts = field_or_opts
+      if opts[:label]
+        Deprecation.warn self, "The second argument to link_to_document should now be the label."
+        field = opts.delete(:label)
+      end
+    else
+      field = field_or_opts
+    end
+
+    field ||= document_show_link_field(doc)
+    label = presenter(doc).render_document_index_label field, opts
     link_to label, url_for_document(doc), document_link_params(doc, opts)
   end
 

app/views/catalog/_index_header_default.html.erb

@@ -11,7 +11,7 @@
          <span class="document-counter">
          <%= t('blacklight.search.documents.counter', :counter => counter) if counter %>
          </span>
-         <%= link_to_document document, :label=>document_show_link_field(document), :counter => counter %>
+         <%= link_to_document document, document_show_link_field(document), :counter => counter %>
         </h5>
 
         <% # bookmark functions for items/docs -%>

app/views/catalog/_show_more_like_this.html.erb

@@ -1,3 +1,3 @@
 <li class="more_like_this_document">
-  <span class="mlt_title"><%= link_to_document document, :label=>document_show_link_field(document) %></span>
-</li>
+  <span class="mlt_title"><%= link_to_document document, document_show_link_field(document) %></span>
+</li>

lib/blacklight/document_presenter.rb

@@ -2,6 +2,7 @@ module Blacklight
   class DocumentPresenter
     include ActionView::Helpers::OutputSafetyHelper
     include ActionView::Helpers::TagHelper
+    extend Deprecation
 
     # @param [SolrDocument] document
     # @param [ActionController::Base] controller scope for linking and generating urls
@@ -55,17 +56,25 @@ def render_field_value value=nil, field_config=nil
     ##
     # Render the document index heading
     #
-    # @param [Hash] opts
+    # @param [Hash] opts (Deprecated)
     # @option opts [Symbol] :label Render the given field from the document
     # @option opts [Proc] :label Evaluate the given proc
     # @option opts [String] :label Render the given string
-    def render_document_index_label opts = {}
-      label = nil
-      label ||= @document.get(opts[:label], :sep => nil) if opts[:label].instance_of? Symbol
-      label ||= opts[:label].call(@document, opts) if opts[:label].instance_of? Proc
-      label ||= opts[:label] if opts[:label].is_a? String
-      label ||= @document.id
-      render_field_value label
+    # @param [Symbol, Proc, String] field Render the given field or evaluate the proc or render the given string
+    def render_document_index_label field, opts ={}
+      if field.kind_of? Hash
+        Deprecation.warn DocumentPresenter, "Calling render_document_index_label with a hash is deprecated"
+        field = field[:label]
+      end
+      label = case field
+      when Symbol
+        @document.get(field, :sep => nil)
+      when Proc
+        field.call(@document, opts)
+      when String
+        field
+      end
+      render_field_value label || @document.id
     end
 
     ##

spec/helpers/catalog_helper_spec.rb

@@ -11,7 +11,7 @@ def mock_response args
     start = (current_page - 1) * per_page
 
     mock_docs = (1..total).to_a.map { {}.with_indifferent_access }
-    
+
     mock_response = Kaminari.paginate_array(mock_docs).page(current_page).per(per_page)
 
     allow(mock_response).to receive(:docs).and_return(mock_docs.slice(start, per_page))
@@ -21,8 +21,8 @@ def mock_response args
   def render_grouped_response?
     false
   end
-  
-  
+
+
   describe "page_entries_info" do
     before(:all) do
     end
@@ -184,17 +184,17 @@ def render_grouped_response?
       allow(helper).to receive_messages(:blacklight_config => Blacklight::Configuration.new(:index => Blacklight::OpenStructWithHashAccess.new(:thumbnail_method => :xyz) ))
       allow(helper).to receive_messages(:xyz => "some-thumbnail")
 
-      allow(helper).to receive(:link_to_document).with(document, :label => "some-thumbnail")
+      allow(helper).to receive(:link_to_document).with(document, "some-thumbnail", {})
       helper.render_thumbnail_tag document
     end
 
     it "should create an image tag from the given field" do
       allow(helper).to receive_messages(:blacklight_config => Blacklight::Configuration.new(:index => Blacklight::OpenStructWithHashAccess.new(:thumbnail_field => :xyz) ))
-      
+
       allow(document).to receive(:has?).with(:xyz).and_return(true)
       allow(document).to receive(:first).with(:xyz).and_return("http://example.com/some.jpg")
 
-      allow(helper).to receive(:link_to_document).with(document, :label => image_tag("http://example.com/some.jpg"))
+      allow(helper).to receive(:link_to_document).with(document, image_tag("http://example.com/some.jpg"), {})
       helper.render_thumbnail_tag document
     end
 
@@ -213,7 +213,7 @@ def render_grouped_response?
       result = helper.render_thumbnail_tag document, {}, suppress_link: true
       expect(result).to eq "some-thumbnail"
     end
-    
+
 
     it "should return nil if no thumbnail is available" do
       allow(helper).to receive_messages(:blacklight_config => Blacklight::Configuration.new(:index => Blacklight::OpenStructWithHashAccess.new() ))
@@ -223,7 +223,7 @@ def render_grouped_response?
     it "should return nil if no thumbnail is returned from the thumbnail method" do
       allow(helper).to receive_messages(:blacklight_config => Blacklight::Configuration.new(:index => Blacklight::OpenStructWithHashAccess.new(:thumbnail_method => :xyz) ))
       allow(helper).to receive_messages(:xyz => nil)
-      
+
       expect(helper.render_thumbnail_tag document).to be_nil
     end
   end

spec/helpers/url_helper_spec.rb

@@ -254,37 +254,44 @@
     it "should consist of the document title wrapped in a <a>" do
       data = {'id'=>'123456','title_display'=>['654321'] }
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => :title_display })).to have_selector("a", :text => '654321', :count => 1)
+      expect(helper.link_to_document(@document, :title_display)).to have_selector("a", :text => '654321', :count => 1)
     end
 
-    it "should accept and return a string label" do
+    it "should have the old deprecated behavior (second argument is a hash)" do
       data = {'id'=>'123456','title_display'=>['654321'] }
       @document = SolrDocument.new(data)
+      expect(Deprecation).to receive(:warn)
       expect(helper.link_to_document(@document, { :label => "title_display" })).to have_selector("a", :text => 'title_display', :count => 1)
     end
 
+    it "should accept and return a string label" do
+      data = {'id'=>'123456','title_display'=>['654321'] }
+      @document = SolrDocument.new(data)
+      expect(helper.link_to_document(@document, "title_display")).to have_selector("a", :text => 'title_display', :count => 1)
+    end
+
     it "should accept and return a Proc" do
       data = {'id'=>'123456','title_display'=>['654321'] }
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => Proc.new { |doc, opts| doc.get(:id) + ": " + doc.get(:title_display) } })).to have_selector("a", :text => '123456: 654321', :count => 1)
+      expect(helper.link_to_document(@document, Proc.new { |doc, opts| doc.get(:id) + ": " + doc.get(:title_display) })).to have_selector("a", :text => '123456: 654321', :count => 1)
     end
 
     it "should return id when label is missing" do
       data = {'id'=>'123456'}
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => :title_display })).to have_selector("a", :text => '123456', :count => 1)
+      expect(helper.link_to_document(@document, :title_display)).to have_selector("a", :text => '123456', :count => 1)
     end
 
     it "should be html safe" do
       data = {'id'=>'123456'}
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => :title_display })).to be_html_safe
+      expect(helper.link_to_document(@document, :title_display)).to be_html_safe
     end
 
     it "should convert the counter parameter into a data- attribute" do
       data = {'id'=>'123456','title_display'=>['654321']}
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => :title_display, :counter => 5  })).to match /\/catalog\/123456\/track\?counter=5/
+      expect(helper.link_to_document(@document, :title_display, counter: 5)).to match /\/catalog\/123456\/track\?counter=5/
     end
 
     it "should merge the data- attributes from the options with the counter params" do
@@ -297,12 +304,12 @@
 
     it "passes on the title attribute to the link_to_with_data method" do
       @document = SolrDocument.new('id'=>'123456')
-      expect(helper.link_to_document(@document,:label=>"Some crazy long label...",:title=>"Some crazy longer label")).to match(/title=\"Some crazy longer label\"/)
+      expect(helper.link_to_document(@document, "Some crazy long label...", title: "Some crazy longer label")).to match(/title=\"Some crazy longer label\"/)
     end
 
     it "doesn't add an erroneous title attribute if one isn't provided" do
       @document = SolrDocument.new('id'=>'123456')
-      expect(helper.link_to_document(@document,:label=>"Some crazy long label...")).to_not match(/title=/)
+      expect(helper.link_to_document(@document, "Some crazy long label...")).to_not match(/title=/)
     end
 
     it "should  work with integer ids" do