Fixes #15257 - let print adapters decide whether to paginate

lib/hammer_cli_foreman/commands.rb

@@ -328,7 +328,9 @@ def pagination_supported?
     end
 
     def should_retrieve_all?
-      pagination_supported? && option_per_page.nil? && option_page.nil? && HammerCLI::Settings.get(:ui, :per_page).nil?
+      retrieve_all = pagination_supported? && option_per_page.nil? && option_page.nil?
+      retrieve_all &&= HammerCLI::Settings.get(:ui, :per_page).nil? if output.adapter.paginate_by_default?
+      retrieve_all
     end
 
   end

test/functional/commands/list_test.rb

@@ -1,50 +1,139 @@
 require File.join(File.dirname(__FILE__), '..', 'test_helper')
 
-describe "ListCommand" do
-  describe "pagination" do
-    let(:row1) { { :id => '1', :name => 'Name', :type => 'Type' } }
-    let(:row2) { { :id => '2', :name => 'Name', :type => 'Type' } }
-    let(:header) { ['ID', 'NAME', 'TYPE'] }
+describe HammerCLIForeman::ListCommand do
+  class TestList < HammerCLIForeman::ListCommand
+    resource :domains
+
+    option '--page', 'PAGE', ''
+    option '--per-page', 'PER_PAGE', ''
+  end
+
+  class TestListWithOutput < TestList
+    output do
+      field :id, "Id"
+      field :name, "Name"
+    end
+  end
+
+  def build_items(cnt)
+    (1..cnt).map do |i|
+      {:id => i, :name => "Item #{i}"}
+    end
+  end
+
+  def expect_paged_call(page, per_page, item_cnt, response_metadata={})
+    api_expects(:domains, :index, "List records page #{page}") do |par|
+      par["page"].to_i == page && par["per_page"].to_i == per_page
+    end.returns(index_response(build_items(item_cnt), response_metadata))
+  end
+
+  let(:per_page_all) { HammerCLIForeman::ListCommand::RETRIEVE_ALL_PER_PAGE }
+
+  after do
+    HammerCLI::Settings.clear
+  end
+
+  describe "api interaction" do
+    context "without per_page in settings" do
+      it "fetches only first page when there's not enough records" do
+        expect_paged_call(1, 1000, 10)
+        result = run_cmd([], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "fetches all records" do
+        expect_paged_call(1, per_page_all, 1000)
+        expect_paged_call(2, per_page_all, 1000)
+        expect_paged_call(3, per_page_all, 10)
+
+        result = run_cmd([], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "uses --per-page value" do
+        per_page = 10
+        expect_paged_call(1, per_page, 10)
+        result = run_cmd(["--per-page=#{per_page}"], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "uses both --per-page and --page value" do
+        per_page = 10
+        expect_paged_call(2, per_page, 10)
+        result = run_cmd(["--per-page=#{per_page}", '--page=2'], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "sets per_page to 20 when only --page is used" do
+        expect_paged_call(2, 20, 10)
+        result = run_cmd(['--page=2'], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+    end
+
+    context "with per_page in settings" do
+      let(:per_page_in_settings) { 30 }
+      before do
+        HammerCLI::Settings.load({ :ui => { :per_page => per_page_in_settings } })
+      end
+
+      it "gives preference to --per-page option over per_page setting" do
+        per_page = 10
+        expect_paged_call(1, per_page, 10)
+        result = run_cmd(["--per-page=#{per_page}"], {}, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "respects per_page setting when the adapter allows pagination by default" do
+        expect_paged_call(1, per_page_in_settings, 30)
+        result = run_cmd([], { :adapter => :base, :interactive => false }, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+
+      it "fetches all records when the adapter doesn't allow pagination by default" do
+        expect_paged_call(1, per_page_all, 1000)
+        expect_paged_call(2, per_page_all, 10)
+        result = run_cmd([], { :adapter => :csv, :interactive => false }, TestList)
+        result.exit_code.must_equal HammerCLI::EX_OK
+      end
+    end
+  end
+
+  describe "pagination output" do
+    let(:row_data) { ['1', 'Item 1'] }
+    let(:header) { ['ID', 'NAME'] }
     let(:pagination) { ['Page 1 of 2 (use --page and --per-page for navigation)'] }
-    let(:output_without_pagination) { IndexMatcher.new([header, row1.values]) }
-    let(:output_with_pagination) { IndexMatcher.new([header, row1.values, pagination]) }
+    let(:output_without_pagination) { IndexMatcher.new([header, row_data]) }
+    let(:output_with_pagination) { IndexMatcher.new([header, row_data, pagination]) }
+    let(:pagination_line_re) { /Page [1-9] of [0-9]/ }
 
     it 'prints all rows by default' do
       expected_result = success_result(output_without_pagination)
-      api_expects(:config_templates, :index, "List all parameters") do |par|
-        par["page"] == 1 && par["per_page"] == 1000
-      end.returns(index_response([row1]))
+      expect_paged_call(1, per_page_all, 1)
 
-      result = run_cmd(['template', 'list'], {})
+      result = run_cmd([], {}, TestListWithOutput)
       assert_cmd(expected_result, result)
-      result.out.wont_match /Page [1-9] of [0-9]/
+      result.out.wont_match pagination_line_re
     end
 
     it 'prints one page when --per-page is used' do
       expected_result = success_result(output_with_pagination)
-      api_expects(:config_templates, :index, "List 1st page") do |par|
-        par["page"].to_i == 1 && par["per_page"].to_i == 1
-      end.returns(index_response([row1], :total => 2, :subtotal => 2, :per_page => 1, :page => 1))
+      expect_paged_call(1, 1, 1, :total => 2, :subtotal => 2, :per_page => 1, :page => 1)
 
-      result = run_cmd(['template', 'list', '--per-page=1'], {})
+      result = run_cmd(['--per-page=1'], {}, TestListWithOutput)
       assert_cmd(expected_result, result)
     end
 
-    context 'in settings' do
-      before :all do
+    context 'with per_page in settings' do
+      before do
         HammerCLI::Settings.load({ :ui => { :per_page => '1' } })
       end
-      after :all do
-        HammerCLI::Settings.load({ :ui => { :per_page => nil } })
-      end
 
       it 'prints one page when per_page is set in the config' do
         expected_result = success_result(output_with_pagination)
-        api_expects(:config_templates, :index, "List 1st page") do |par|
-          par["page"].to_i == 1 && par["per_page"].to_i == 1
-        end.returns(index_response([row1], :total => 2, :subtotal => 2, :per_page => 1, :page => 1))
+        expect_paged_call(1, 1, 1, :total => 2, :subtotal => 2, :per_page => 1, :page => 1)
 
-        result = run_cmd(['template', 'list'], {})
+        result = run_cmd([], {}, TestListWithOutput)
         assert_cmd(expected_result, result)
       end
     end

test/unit/commands_test.rb

@@ -143,17 +143,17 @@ class Assoc < HammerCLIForeman::AddAssociatedCommand
     builder.class.must_equal HammerCLIForeman::ForemanOptionBuilder
   end
 
-  it "properly raises error on intentional searching of parameters that are not required" do 
+  it "properly raises error on intentional searching of parameters that are not required" do
     class TestList < HammerCLIForeman::ListCommand
       resource :domains
       build_options
     end
 
     com = TestList.new("", { :adapter => :csv, :interactive => false })
-    
+
     com.resolver.class.any_instance.stubs(:location_id).raises(
       HammerCLIForeman::MissingSearchOptions.new(
-        "Error", 
+        "Error",
         HammerCLIForeman.foreman_api_connection.api.resource(:locations)
       )
     )
@@ -164,21 +164,21 @@ class TestList < HammerCLIForeman::ListCommand
 
   end
 
-  it "ignores error on attempt to search of parameters that are not required" do 
+  it "ignores error on attempt to search of parameters that are not required" do
     class TestList < HammerCLIForeman::ListCommand
       resource :domains
       build_options
     end
 
     com = TestList.new("", { :adapter => :csv, :interactive => false })
-    
+
     com.resolver.class.any_instance.stubs(:location_id).raises(
       HammerCLIForeman::MissingSearchOptions.new(
-        "Error", 
+        "Error",
         HammerCLIForeman.foreman_api_connection.api.resource(:locations)
       )
     )
-    
+
     out, err = capture_io do
       com.run([]).must_equal HammerCLI::EX_OK
     end