Merge pull request #933 from projectblacklight/render_facet_limit_list

Move logic from view into a helper: render_facet_limit_list

Author: cbeer

app/helpers/blacklight/facets_helper_behavior.rb

@@ -50,6 +50,27 @@ def render_facet_limit(display_facet, options = {})
     render(options)
   end
 
+  ##
+  # Renders the list of values 
+  # removes any elements where render_facet_item returns a nil value. This enables an application
+  # to filter undesireable facet items so they don't appear in the UI
+  def render_facet_limit_list(paginator, solr_field, wrapping_element=:li)
+    safe_join(paginator.items.
+      map { |item| render_facet_item(solr_field, item) }.compact.
+      map { |item| content_tag(wrapping_element,item)}
+    )
+  end
+
+  ##
+  # Renders a single facet item
+  def render_facet_item(solr_field, item)
+    if facet_in_params?( solr_field, item.value )
+      render_selected_facet_value(solr_field, item)          
+    else
+      render_facet_value(solr_field, item)
+    end
+  end
+
   ##
   # Determine if Blacklight should render the display_facet or not
   #
@@ -77,7 +98,10 @@ def should_collapse_facet? facet_field
   end
 
   ##
-  # the name of the partial to use to render a facet field.
+  # The name of the partial to use to render a facet field.
+  # uses the value of the "partial" field if set in the facet configuration
+  # otherwise uses "facet_pivot" if this facet is a pivot facet 
+  # defaults to 'facet_limit'
   #
   # @return [String]
   def facet_partial_name(display_facet = nil)

app/views/catalog/_facet_limit.html.erb

@@ -1,18 +1,9 @@
 <ul class="facet-values list-unstyled">
-  <% paginator = facet_paginator(facet_field, display_facet)
-  %>
-  <% paginator.items.each do |item| -%>
-    <li>
-    <% if facet_in_params?( solr_field, item.value ) %>              
-        <%= render_selected_facet_value(solr_field, item) %>              
-      <% else %>              
-        <%= render_facet_value(solr_field, item) %>              
-    <% end -%>
-    </li>
-  <% end %>
+  <% paginator = facet_paginator(facet_field, display_facet) %>
+  <%= render_facet_limit_list paginator, solr_field %>
 
   <% unless paginator.last_page? || params[:action] == "facet" %>
-    <li class="more_facets_link"><%= link_to(t('blacklight.search.facets.more'), params.merge(:id => solr_field, :action=>"facet", :page => nil), :class => "more_facets_link") %></li>
+    <li class="more_facets_link"><%= link_to t('blacklight.search.facets.more'), 
+       params.merge(id: solr_field, action: "facet", page: nil), class: "more_facets_link" %></li>
   <% end %>
-
 </ul>

spec/helpers/facets_helper_spec.rb

@@ -17,15 +17,15 @@
       empty = double(:items => [])
 
       fields = [a,b,empty]
-      expect(helper.has_facet_values?(fields)).to be_true
+      expect(helper.has_facet_values?(fields)).to be true
     end
 
     it "should be false if all facets are empty" do
 
       empty = double(:items => [])
 
       fields = [empty]
-      expect(helper.has_facet_values?(fields)).to be_false
+      expect(helper.has_facet_values?(fields)).to be false
     end
   end
 
@@ -45,37 +45,37 @@
 
     it "should render facets with items" do
       a = double(:items => [1,2], :name=>'basic_field')
-      expect(helper.should_render_facet?(a)).to be_true
+      expect(helper.should_render_facet?(a)).to be true
     end
     it "should not render facets without items" do
       empty = double(:items => [], :name=>'basic_field')
-      expect(helper.should_render_facet?(empty)).to be_false
+      expect(helper.should_render_facet?(empty)).to be false
     end
 
     it "should not render facets where show is set to false" do
       a = double(:items => [1,2], :name=>'no_show')
-      expect(helper.should_render_facet?(a)).to be_false
+      expect(helper.should_render_facet?(a)).to be false
     end
 
     it "should call a helper to determine if it should render a field" do
       helper.stub(:my_helper => true)
       a = double(:items => [1,2], :name=>'helper_show')
-      expect(helper.should_render_facet?(a)).to be_true
+      expect(helper.should_render_facet?(a)).to be true
     end
 
     it "should call a helper to determine if it should render a field" do
       a = double(:items => [1,2], :name=>'helper_with_an_arg_show')
       helper.should_receive(:my_helper_with_an_arg).with(@config.facet_fields['helper_with_an_arg_show'], a).and_return(true)
-      expect(helper.should_render_facet?(a)).to be_true
+      expect(helper.should_render_facet?(a)).to be true
     end
 
 
     it "should evaluate a Proc to determine if it should render a field" do
       a = double(:items => [1,2], :name=>'lambda_show')
-      expect(helper.should_render_facet?(a)).to be_true
+      expect(helper.should_render_facet?(a)).to be true
 
       a = double(:items => [1,2], :name=>'lambda_no_show')
-      expect(helper.should_render_facet?(a)).to be_false
+      expect(helper.should_render_facet?(a)).to be false
     end
   end
 
@@ -90,17 +90,17 @@
     end
 
     it "should be collapsed by default" do
-      expect(helper.should_collapse_facet?(@config.facet_fields['basic_field'])).to be_true
+      expect(helper.should_collapse_facet?(@config.facet_fields['basic_field'])).to be true
     end
 
     it "should not be collapsed if the configuration says so" do
-      expect(helper.should_collapse_facet?(@config.facet_fields['no_collapse'])).to be_false
+      expect(helper.should_collapse_facet?(@config.facet_fields['no_collapse'])).to be false
     end
 
     it "should not be collapsed if it is in the params" do
       params[:f] = { basic_field: [1], no_collapse: [2] }.with_indifferent_access
-      expect(helper.should_collapse_facet?(@config.facet_fields['basic_field'])).to be_false
-      expect(helper.should_collapse_facet?(@config.facet_fields['no_collapse'])).to be_false
+      expect(helper.should_collapse_facet?(@config.facet_fields['basic_field'])).to be false
+      expect(helper.should_collapse_facet?(@config.facet_fields['no_collapse'])).to be false
     end
 
   end
@@ -152,7 +152,7 @@
         field.should be_a_kind_of Blacklight::SolrResponse::Facets::FacetField
 
         expect(field.name).to eq'my_query_facet_field'
-        expect(field.items).to have(2).items
+        expect(field.items.size).to eq 2
         expect(field.items.map { |x| x.value }).to_not include 'field:not_appearing_in_the_config'
 
         facet_item = field.items.select { |x| x.value == 'a_simple_query' }.first
@@ -180,11 +180,11 @@
 
         expect(field.name).to eq 'my_pivot_facet_field'
 
-        expect(field.items).to have(1).item
+        expect(field.items.size).to eq 1
 
         expect(field.items.first).to respond_to(:items)
 
-        expect(field.items.first.items).to have(1).item
+        expect(field.items.first.items.size).to eq 1
         expect(field.items.first.items.first.fq).to eq({ 'field_a' => 'a' })
       end
     end
@@ -281,11 +281,40 @@
     end 
   end
 
+  describe "render_facet_limit_list" do
+    let(:f1) { Blacklight::SolrResponse::Facets::FacetItem.new(hits: '792', value: 'Book') }
+    let(:f2) { Blacklight::SolrResponse::Facets::FacetItem.new(hits: '65', value: 'Musical Score') }
+    let(:paginator) { Blacklight::Solr::FacetPaginator.new([f1, f2], limit: 10) }
+    subject { helper.render_facet_limit_list(paginator, 'type_solr_field') }
+    before do
+      allow(helper).to receive(:search_action_path) do |*args|
+        catalog_index_path *args
+      end
+    end
+    it "should draw a list of elements" do
+      expect(subject).to have_selector 'li', count: 2
+      expect(subject).to have_selector 'li:first-child a.facet_select', text: 'Book' 
+      expect(subject).to have_selector 'li:nth-child(2) a.facet_select', text: 'Musical Score' 
+    end
+
+    context "when one of the facet items is rendered as nil" do
+      # An app may override render_facet_item to filter out some undesired facet items by returning nil.
+      
+      before { allow(helper).to receive(:render_facet_item).and_return("<a class=\"facet_select\">Book</a>".html_safe, nil) }
+
+      it "should draw a list of elements" do
+        expect(subject).to have_selector 'li', count: 1
+        expect(subject).to have_selector 'li:first-child a.facet_select', text: 'Book' 
+      end
+
+    end
+  end
+
   describe "facet_field_in_params?" do
     it "should check if the facet field is selected in the user params" do
       helper.stub(:params => { :f => { "some-field" => ["x"]}})
-      expect(helper.facet_field_in_params?("some-field")).to be_true
-      expect(helper.facet_field_in_params?("other-field")).to_not be_true
+      expect(helper.facet_field_in_params?("some-field")).to be_truthy
+      expect(helper.facet_field_in_params?("other-field")).to_not be true
     end
   end
 
@@ -300,7 +329,7 @@
       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|
+      allow(helper).to receive(:search_action_path) do |*args|
         catalog_index_path *args
       end
     end