Merge pull request #1453 from projectblacklight/search-state

Update Blacklight::SearchState to play well with Rails 5 AC::Parameters class

Author: jcoyne

lib/blacklight/search_state.rb

@@ -10,22 +10,25 @@ class SearchState
     # @param [ActionController::Parameters] params
     # @param [Blacklight::Config] blacklight_config
     def initialize(params, blacklight_config)
-      if params.instance_of? Hash
-        # This is an ActionView::TestCase workaround. Will be resolved by
-        # https://github.com/rails/rails/pull/22913 (Rails > 4.2.5)
-        @params = params.with_indifferent_access
-      else
+      if params.respond_to?(:to_unsafe_h)
         # This is the typical (not-ActionView::TestCase) code path.
         @params = params.to_unsafe_h
         # In Rails 5 to_unsafe_h returns a HashWithIndifferentAccess, in Rails 4 it returns Hash
         @params = @params.with_indifferent_access if @params.instance_of? Hash
+      elsif params.is_a? Hash
+        # This is an ActionView::TestCase workaround for Rails 4.2.
+        @params = params.dup.with_indifferent_access
+      else
+        @params = params.dup.to_h.with_indifferent_access
       end
+
       @blacklight_config = blacklight_config
     end
 
-    def to_h
+    def to_hash
       @params
     end
+    alias to_h to_hash
 
     def reset
       Blacklight::SearchState.new(ActionController::Parameters.new, blacklight_config)
@@ -112,7 +115,7 @@ def remove_facet_params(field, item)
     # @yield [params] The merged parameters hash before being sanitized
     def params_for_search(params_to_merge={}, &block)
       # params hash we'll return
-      my_params = params.dup.merge(params_to_merge.dup)
+      my_params = params.dup.merge(Blacklight::SearchState.new(params_to_merge, blacklight_config))
 
       if block_given?
         yield my_params

spec/lib/blacklight/search_state_spec.rb

@@ -9,27 +9,61 @@
   end
 
   let(:parameter_class) { ActionController::Parameters }
-  let(:helper) { described_class.new(params, blacklight_config) }
+  let(:search_state) { described_class.new(params, blacklight_config) }
   let(:params) { parameter_class.new }
 
+  describe '#to_h' do
+    let(:data) { { a: '1'} }
+    let(:params) { parameter_class.new data }
+
+    it 'returns a copy of the original parameters' do
+      expect(search_state.to_h).to eq data.with_indifferent_access
+      expect(search_state.to_h.object_id).not_to eq params.object_id
+    end
+
+    context 'with AC::Parameters' do
+      let(:parameter_class) { ActionController::Parameters }
+
+      it 'returns the hash data' do
+        expect(search_state.to_h).to eq data.with_indifferent_access
+      end
+    end
+
+    context 'with HashWithIndifferentAccess' do
+      let(:parameter_class) { HashWithIndifferentAccess }
+
+      it 'returns the hash data' do
+        expect(search_state.to_h).to eq data.with_indifferent_access
+      end
+    end
+
+    context 'with Hash' do
+      let(:params) { data }
+
+      it 'returns the hash data' do
+        expect(search_state.to_h).to eq data.with_indifferent_access
+      end
+    end
+  end
+
   describe "params_for_search" do
     let(:params) { parameter_class.new 'default' => 'params' }
 
     it "takes original params" do
-      result = helper.params_for_search
+      result = search_state.params_for_search
       expect(result).to eq({ 'default' => 'params' })
       expect(params.object_id).to_not eq result.object_id
     end
 
     it "accepts params to merge into the controller's params" do
-      result = helper.params_for_search(q: 'query')
+      result = search_state.params_for_search(q: 'query')
       expect(result).to eq('q' => 'query', 'default' => 'params')
     end
 
     context "when params have blacklisted keys" do
       let(:params) { parameter_class.new action: 'action', controller: 'controller', id: "id", commit: 'commit' }
       it "removes them" do
-        result = helper.params_for_search
+        result = search_state.params_for_search
         expect(result.keys).to_not include(:action, :controller, :commit, :id)
       end
     end
@@ -38,31 +72,31 @@
       context "and per_page changed" do
         let(:params) { parameter_class.new per_page: 20, page: 5 }
         it "adjusts the current page" do
-          result = helper.params_for_search(per_page: 100)
+          result = search_state.params_for_search(per_page: 100)
           expect(result[:page]).to eq 1
         end
       end
 
       context "and per_page didn't change" do
         let(:params) { parameter_class.new per_page: 20, page: 5 }
         it "doesn't change the current page" do
-          result = helper.params_for_search(per_page: 20)
+          result = search_state.params_for_search(per_page: 20)
           expect(result[:page]).to eq 5
         end
       end
 
       context "and the sort changes" do
         let(:params) { parameter_class.new sort: 'field_a', page: 5 }
         it "adjusts the current page" do
-          result = helper.params_for_search(sort: 'field_b')
+          result = search_state.params_for_search(sort: 'field_b')
           expect(result[:page]).to eq 1
         end
       end
 
       context "and the sort didn't change" do
         let(:params) { parameter_class.new sort: 'field_a', page: 5 }
         it "doesn't change the current page" do
-          result = helper.params_for_search(sort: 'field_a')
+          result = search_state.params_for_search(sort: 'field_a')
           expect(result[:page]).to eq 5
         end
       end
@@ -71,7 +105,7 @@
     context "with a block" do
       let(:params) { parameter_class.new a: 1, b: 2 }
       it "evalutes the block and allow it to add or remove keys" do
-        result = helper.params_for_search(c: 3) do |params|
+        result = search_state.params_for_search(c: 3) do |params|
           params.extract! :a, :b
           params[:d] = 'd'
         end
@@ -100,14 +134,14 @@
     context "when there are no pre-existing facets" do
       let(:params) { params_no_existing_facet }
       it "adds facet value" do
-        result_params = helper.add_facet_params("facet_field", "facet_value")
+        result_params = search_state.add_facet_params("facet_field", "facet_value")
         expect(result_params[:f]).to be_a Hash
         expect(result_params[:f]["facet_field"]).to be_a_kind_of(Array)
         expect(result_params[:f]["facet_field"]).to eq ["facet_value"]
       end
 
       it "leaves non-facet params alone" do
-        result_params = helper.add_facet_params("facet_field_2", "new_facet_value")
+        result_params = search_state.add_facet_params("facet_field_2", "new_facet_value")
 
         params.each_pair do |key, value|
           next if key == :f
@@ -116,9 +150,9 @@
       end
 
       it "uses the facet's key in the url" do
-        allow(helper).to receive(:facet_configuration_for_field).with('some_field').and_return(double(single: true, field: "a_solr_field", key: "some_key"))
+        allow(search_state).to receive(:facet_configuration_for_field).with('some_field').and_return(double(single: true, field: "a_solr_field", key: "some_key"))
 
-        result_params = helper.add_facet_params('some_field', 'my_value')
+        result_params = search_state.add_facet_params('some_field', 'my_value')
 
         expect(result_params[:f]['some_key']).to have(1).item
         expect(result_params[:f]['some_key'].first).to eq 'my_value'
@@ -128,7 +162,7 @@
     context "when there are pre-existing facets" do
       let(:params) { params_existing_facets }
       it "adds a facet param to existing facet constraints" do
-        result_params = helper.add_facet_params("facet_field_2", "new_facet_value")
+        result_params = search_state.add_facet_params("facet_field_2", "new_facet_value")
 
         expect(result_params[:f]).to be_a Hash
 
@@ -143,7 +177,7 @@
       end
 
       it "leaves non-facet params alone" do
-        result_params = helper.add_facet_params("facet_field_2", "new_facet_value")
+        result_params = search_state.add_facet_params("facet_field_2", "new_facet_value")
 
         params.each_pair do |key, value|
           next if key == 'f'
@@ -155,29 +189,29 @@
     context "with a facet selected in the params" do
       let(:params) { parameter_class.new f: { 'single_value_facet_field' => 'other_value' } }
       it "replaces facets configured as single" do
-        allow(helper).to receive(:facet_configuration_for_field).with('single_value_facet_field').and_return(double(single: true, key: "single_value_facet_field"))
-        result_params = helper.add_facet_params('single_value_facet_field', 'my_value')
+        allow(search_state).to receive(:facet_configuration_for_field).with('single_value_facet_field').and_return(double(single: true, key: "single_value_facet_field"))
+        result_params = search_state.add_facet_params('single_value_facet_field', 'my_value')
 
         expect(result_params[:f]['single_value_facet_field']).to have(1).item
         expect(result_params[:f]['single_value_facet_field'].first).to eq 'my_value'
       end
     end
 
     it "accepts a FacetItem instead of a plain facet value" do
-      result_params = helper.add_facet_params('facet_field_1', double(value: 123))
+      result_params = search_state.add_facet_params('facet_field_1', double(value: 123))
 
       expect(result_params[:f]['facet_field_1']).to include(123)
     end
 
     it "defers to the field set on a FacetItem" do
-      result_params = helper.add_facet_params('facet_field_1', double(:field => 'facet_field_2', :value => 123))
+      result_params = search_state.add_facet_params('facet_field_1', double(:field => 'facet_field_2', :value => 123))
 
       expect(result_params[:f]['facet_field_1']).to be_blank
       expect(result_params[:f]['facet_field_2']).to include(123)
     end
 
     it "adds any extra fq parameters from the FacetItem" do
-      result_params = helper.add_facet_params('facet_field_1', double(:value => 123, fq: { 'facet_field_2' => 'abc' }))
+      result_params = search_state.add_facet_params('facet_field_1', double(:value => 123, fq: { 'facet_field_2' => 'abc' }))
 
       expect(result_params[:f]['facet_field_1']).to include(123)
       expect(result_params[:f]['facet_field_2']).to include('abc')
@@ -197,7 +231,7 @@
     }
 
     it "does not include request parameters used by the facet paginator" do
-      params = helper.add_facet_params_and_redirect("facet_field_2", "facet_value")
+      params = search_state.add_facet_params_and_redirect("facet_field_2", "facet_value")
 
       bad_keys = Blacklight::Solr::FacetPaginator.request_keys.values + [:id]
       bad_keys.each do |paginator_key|
@@ -206,13 +240,13 @@
     end
 
     it 'removes :page request key' do
-      params = helper.add_facet_params_and_redirect("facet_field_2", "facet_value")
+      params = search_state.add_facet_params_and_redirect("facet_field_2", "facet_value")
       expect(params).to_not have_key(:page)
     end
 
     it "otherwise does the same thing as add_facet_params" do
-      added_facet_params = helper.add_facet_params("facet_field_2", "facet_value")
-      added_facet_params_from_facet_action = helper.add_facet_params_and_redirect("facet_field_2", "facet_value")
+      added_facet_params = search_state.add_facet_params("facet_field_2", "facet_value")
+      added_facet_params_from_facet_action = search_state.add_facet_params_and_redirect("facet_field_2", "facet_value")
 
       expect(added_facet_params_from_facet_action).to eq added_facet_params.except(Blacklight::Solr::FacetPaginator.request_keys[:page], Blacklight::Solr::FacetPaginator.request_keys[:sort])
     end
@@ -225,21 +259,21 @@
       let(:facet_params) { { 'some_field' => ['some_value', 'another_value'] } }
 
       it "removes the facet / value tuple from the query parameters" do
-        params = helper.remove_facet_params('some_field', 'some_value')
+        params = search_state.remove_facet_params('some_field', 'some_value')
         expect(params[:f]['some_field']).not_to include 'some_value'
         expect(params[:f]['some_field']).to include 'another_value'
       end
     end
 
     context "when the facet has configuration" do
       before do
-        allow(helper).to receive(:facet_configuration_for_field).with('some_field').and_return(
+        allow(search_state).to receive(:facet_configuration_for_field).with('some_field').and_return(
           double(single: true, field: "a_solr_field", key: "some_key"))
       end
       let(:facet_params) { { 'some_key' => ['some_value', 'another_value'] } }
 
       it "uses the facet's key configuration" do
-        params = helper.remove_facet_params('some_field', 'some_value')
+        params = search_state.remove_facet_params('some_field', 'some_value')
         expect(params[:f]['some_key']).not_to eq 'some_value'
         expect(params[:f]['some_key']).to include 'another_value'
       end
@@ -249,15 +283,15 @@
       facet_params['another_field'] = ['some_value']
       facet_params['some_field'] = ['some_value']
 
-      params = helper.remove_facet_params('some_field', 'some_value')
+      params = search_state.remove_facet_params('some_field', 'some_value')
 
       expect(params[:f]).not_to have_key 'some_field'
     end
 
     it "removes the 'f' parameter entirely when no facets remain" do
       facet_params['some_field'] = ['some_value']
 
-      params = helper.remove_facet_params('some_field', 'some_value')
+      params = search_state.remove_facet_params('some_field', 'some_value')
 
       expect(params).not_to have_key :f
     end