fix #528; when per_page or sort values are changed, we should always …

…reset the :page parameter to 1

app/helpers/blacklight/blacklight_helper_behavior.rb

@@ -359,24 +359,46 @@ def link_back_to_catalog(opts={:label=>nil})
   end
 
   def params_for_search(options={})
-    my_params = (options[:params] || params).dup
-    options[:omit_keys] ||= []
+    # special keys
+    # params hash to mutate
+    source_params = options.delete(:params) || params
+    omit_keys = options.delete(:omit_keys) || []
 
-    options[:omit_keys].each do |omit_key|
+    # params hash we'll return
+    my_params = source_params.dup.merge(options.dup)
+
+
+    # remove items from our params hash that match:
+    #   - a key
+    #   - a key and a value
+    omit_keys.each do |omit_key|
       case omit_key
         when Hash
           omit_key.each do |key, values|
-            next unless my_params[key]
+            next unless my_params.has_key? key
+
+            # make sure to dup the source key, we don't want to accidentally alter the original
             my_params[key] = my_params[key].dup
 
             values = [values] unless values.respond_to? :each
             values.each { |v| my_params[key].delete(v) }
+
+            if my_params[key].empty?
+              my_params.delete(key)
+            end
           end
+
         else
           my_params.delete(omit_key)
       end
     end
 
+    if my_params[:page] and (my_params[:per_page] != source_params[:per_page] or my_params[:sort] != source_params[:sort] )
+      my_params[:page] = 1
+    end
+
+    my_params.reject! { |k,v| v.nil? }
+
     # removing action and controller from duplicate params so that we don't get hidden fields for them.
     my_params.delete(:action)
     my_params.delete(:controller)

app/views/catalog/_per_page_widget.html.erb

@@ -6,7 +6,7 @@
       <%= link_to(t(:'blacklight.search.per_page.button_label', :count => current_per_page), "#") %> <span class="caret"></span>
       <ul>
         <%- blacklight_config.per_page.each do |count| %>
-          <li><%= link_to(t(:'blacklight.search.per_page.label', :count => count).html_safe, url_for(params_for_search.merge(:per_page => count))) %></li>
+          <li><%= link_to(t(:'blacklight.search.per_page.label', :count => count).html_safe, url_for(params_for_search(:per_page => count))) %></li>
         <%- end -%>
       </ul>
     </li>

app/views/catalog/_sort_widget.html.erb

@@ -5,7 +5,7 @@
       <%= link_to(t('blacklight.search.sort.label', :field =>current_sort_field.label), "#") %> <span class="caret"></span>
       <ul>
         <%- blacklight_config.sort_fields.each do |sort_key, field| %>
-          <li><%= link_to(field.label, url_for(params_for_search.merge(:sort => sort_key))) %></li>
+          <li><%= link_to(field.label, url_for(params_for_search(:sort => sort_key))) %></li>
         <%- end -%>
       </ul>
     </li>

test_support/spec/helpers/blacklight_helper_spec.rb

@@ -146,35 +146,88 @@ def blacklight_config
     end
   end
 
-  describe "search_as_hidden_fields", :asdf => true do
+  describe "params_for_search" do
     def params
-      {:q => "query", :sort => "sort", :per_page => "20", :search_field => "search_field", :page => 100, :arbitrary_key => "arbitrary_value", :f => {"field" => ["value1", "value2"], "other_field" => ['asdf']}, :controller => "catalog", :action => "index", :commit => "search"}
+      { 'default' => 'params'}
     end
-    describe "for default arguments" do
-      it "should default to omitting :page" do
-        search_as_hidden_fields.should have_selector("input[type='hidden']", :count =>8)
-        search_as_hidden_fields.should_not have_selector("input[name='page']")
+
+    it "should default to using the controller's params" do
+      result = params_for_search
+      result.should == params
+      params.object_id.should_not == result.object_id
+    end
+
+    it "should let you pass in params to use" do
+      source_params = { :q => 'query'}
+      result = params_for_search(:params => source_params )
+      source_params.should == result
+      source_params.object_id.should_not == result.object_id
+    end
+
+    it "should not return blacklisted elements" do
+      source_params = { :action => 'action', :controller => 'controller', :commit => 'commit'}
+      result = params_for_search(:params => source_params )
+      result.keys.should_not include(:action, :controller, :commit)
+
+    end
+
+    it "should adjust the current page if the per_page changes" do
+      source_params = { :per_page => 20, :page => 5}
+      result = params_for_search(:params => source_params, :per_page => 100)
+      result[:page].should == 1
+    end
+
+    it "should not adjust the current page if the per_page is the same as it always was" do
+      source_params = { :per_page => 20, :page => 5}
+      result = params_for_search(:params => source_params, :per_page => 20)
+      result[:page].should == 5
+    end
+
+    it "should adjust the current page if the sort changes" do
+      source_params = { :sort => 'field_a', :page => 5}
+      result = params_for_search(:params => source_params, :sort => 'field_b')
+      result[:page].should == 1
+    end
+
+    it "should not adjust the current page if the sort is the same as it always was" do
+      source_params = { :sort => 'field_a', :page => 5}
+      result = params_for_search(:params => source_params, :sort => 'field_a')
+      result[:page].should == 5
+    end
+
+    describe "omit keys parameter" do
+      it "should omit keys by key name" do
+        source_params = { :a => 1, :b => 2, :c => 3}
+        result = params_for_search(:params => source_params, :omit_keys => [:a, :b] )
+
+        result.keys.should_not include(:a, :b)
+        result[:c].should == 3
       end
-      it "should not return blacklisted elements" do
-        search_as_hidden_fields.should_not have_selector("input[name='action']")
-        search_as_hidden_fields.should_not have_selector("input[name='controller']")
-        search_as_hidden_fields.should_not have_selector("input[name='commit']")
+
+      it "should remove keys if a key/value pair was passed and no values are left for that key" do
+        source_params = { :f => ['a']}
+        result = params_for_search(:params => source_params, :omit_keys => [{:f => 'a' }])
+        result.keys.should_not include(:f)
       end
-      describe "for omit_keys parameter" do
-        it "should not include those keys" do
-           generated = search_as_hidden_fields(:omit_keys => [:per_page, :sort])
 
-           generated.should_not have_selector("input[name=sort]")
-           generated.should_not have_selector("input[name=per_page]")
+      it "should only remove keys when a key/value pair is based that are in that pair" do
 
-           generated.should have_selector("input[name=page]")
-        end
+        source_params = { :f => ['a', 'b']}
+        result = params_for_search(:params => source_params, :omit_keys => [{:f => 'a' }])
+        result[:f].should_not include('a')
+        result[:f].should include('b')
+      end
+    end
 
-        it "should support hash-based deleting" do
-           generated = search_as_hidden_fields(:omit_keys => [{:f => 'field' }])
-           generated.should have_selector("input[name='f[other_field][]']")
-           generated.should_not have_selector("input[name='f[field][]']")
-        end
+  end
+
+  describe "search_as_hidden_fields" do
+    def params
+      {:q => "query", :sort => "sort", :per_page => "20", :search_field => "search_field", :page => 100, :arbitrary_key => "arbitrary_value", :f => {"field" => ["value1", "value2"], "other_field" => ['asdf']}, :controller => "catalog", :action => "index", :commit => "search"}
+    end
+    describe "for default arguments" do
+      it "should default to omitting :page" do
+        search_as_hidden_fields.should_not have_selector("input[name='page']")
       end
     end
  end