Address code review feedback

Author: jazairi

README.md

@@ -116,7 +116,7 @@ may have unexpected consequences if applied to other TIMDEX UI apps.
 - `REQUEST_PERIOD` - time in minutes used along with `REQUESTS_PER_PERIOD`
 - `REDIRECT_REQUESTS_PER_PERIOD`- number of requests that can be made that the query string starts with our legacy redirect parameter to throttle per `REQUEST_PERIOD`
 - `REDIRECT_REQUEST_PERIOD`- time in minutes used along with `REDIRECT_REQUEST_PERIOD`
-- `RESULTS_PER_PAGE`: The number of results to display per page. Use an odd number to avoid peculiarities. Defaults to 20 if unset.
+- `RESULTS_PER_PAGE`: The number of results to display per page. Use an even number to avoid peculiarities. Defaults to 20 if unset.
 - `SENTRY_DSN`: Client key for Sentry exception logging.
 - `SENTRY_ENV`: Sentry environment for the application. Defaults to 'unknown' if unset.
 - `TACOS_SOURCE`: If set, this value is sent to TACOS (as the `sourceSystem` value) to distinguish which application

app/controllers/search_controller.rb

@@ -66,7 +66,7 @@ def load_geodata_results
     return unless @errors.nil?
 
     hits = response.dig(:data, 'search', 'hits') || 0
-    @pagination = Analyzer.new(@enhanced_query, { hits: hits }, :timdex).pagination
+    @pagination = Analyzer.new(@enhanced_query, hits, :timdex).pagination
     raw_results = extract_results(response)
     @results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize
     @filters = extract_filters(response)
@@ -98,7 +98,8 @@ def load_all_results
     @results = merge_results(primo_data[:results], timdex_data[:results])
 
     # Use Analyzer for combined pagination calculation
-    @pagination = Analyzer.new(@enhanced_query, { hits: timdex_data[:hits] }, :all, { hits: primo_data[:hits] }).pagination
+    @pagination = Analyzer.new(@enhanced_query, timdex_data[:hits], :all,
+                               primo_data[:hits]).pagination
 
     # Handle primo continuation for high page numbers
     @show_primo_continuation = primo_data[:show_continuation] || false
@@ -124,8 +125,7 @@ def merge_results(primo_results, timdex_results)
   def fetch_primo_data
     current_page = @enhanced_query[:page] || 1
     per_page = if @active_tab == 'all'
-                 ENV.fetch('RESULTS_PER_PAGE',
-                           '20').to_i / 2
+                 ENV.fetch('RESULTS_PER_PAGE', '20').to_i / 2
                else
                  ENV.fetch('RESULTS_PER_PAGE', '20').to_i
                end
@@ -139,7 +139,7 @@ def fetch_primo_data
     primo_response = query_primo(per_page, offset)
     hits = primo_response.dig('info', 'total') || 0
     results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize
-    pagination = Analyzer.new(@enhanced_query, { hits: hits }, :primo).pagination
+    pagination = Analyzer.new(@enhanced_query, hits , :primo).pagination
 
     # Handle empty results from Primo API. Sometimes Primo will return no results at a given offset,
     # despite claiming in the initial query that more are available. This happens randomly and
@@ -183,7 +183,7 @@ def fetch_timdex_data
 
     if errors.nil?
       hits = response.dig(:data, 'search', 'hits') || 0
-      pagination = Analyzer.new(@enhanced_query, { hits: hits }, :timdex).pagination
+      pagination = Analyzer.new(@enhanced_query, hits, :timdex).pagination
       raw_results = extract_results(response)
       results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize
       { results: results, pagination: pagination, errors: nil, hits: hits }

app/models/analyzer.rb

@@ -6,43 +6,52 @@ class Analyzer
   # likely due to performance constraints.
   PRIMO_MAX_OFFSET = 960
 
-  def initialize(enhanced_query, response, source, secondary_response = nil)
+  # Initializes pagination analysis for search results.
+  #
+  # @param enhanced_query [Hash] Query parameters including :page (current page number)
+  # @param hits [Integer] Number of hits from primary source (TIMDEX for :all, source-specific otherwise)
+  # @param source [Symbol] Source tab (:primo, :timdex, or :all)
+  # @param secondary_hits [Integer, nil] Optional hit count from secondary source (Primo hits for :all)
+  def initialize(enhanced_query, hits, source, secondary_hits = nil)
     @source = source
     @enhanced_query = enhanced_query
     @pagination = {}
-    calculate_pagination(response, secondary_response)
+    set_pagination(hits, secondary_hits)
   end
 
   private
 
-  def calculate_pagination(response, secondary_response = nil)
+  # Sets the pagination hash with hit counts and per_page values.
+  #
+  # @param hits [Integer] Hit count from primary source
+  # @param secondary_hits [Integer, nil] Optional hit count from secondary source
+  def set_pagination(hits, secondary_hits = nil)
     if @source == :all
-      @pagination[:hits] = (secondary_response&.dig(:hits) || 0) + (response&.dig(:hits) || 0)
+      @pagination[:hits] = (secondary_hits || 0) + (hits || 0)
       @pagination[:per_page] = ENV.fetch('RESULTS_PER_PAGE', '20').to_i
-      calculate_all_pagination
+      calculate_pagination_values
     else
-      @pagination[:hits] = response&.dig(:hits) || 0
+      @pagination[:hits] = hits || 0
       @pagination[:per_page] = ENV.fetch('RESULTS_PER_PAGE', '20').to_i
-      calculate_standard_pagination
+      calculate_pagination_values
     end
   end
 
-  def calculate_standard_pagination
+  # Calculates and sets pagination navigation values (start, end, prev, next).
+  # Uses the already-set @pagination[:hits] and @pagination[:per_page] values.
+  def calculate_pagination_values
     page = @enhanced_query[:page] || 1
     @pagination[:start] = ((page - 1) * @pagination[:per_page]) + 1
     @pagination[:end] = [page * @pagination[:per_page], @pagination[:hits]].min
     @pagination[:prev] = page - 1 if page > 1
     @pagination[:next] = next_page(page, @pagination[:hits]) if next_page(page, @pagination[:hits])
   end
 
-  def calculate_all_pagination
-    page = @enhanced_query[:page] || 1
-    @pagination[:start] = ((page - 1) * @pagination[:per_page]) + 1
-    @pagination[:end] = [page * @pagination[:per_page], @pagination[:hits]].min
-    @pagination[:prev] = page - 1 if page > 1
-    @pagination[:next] = page + 1 if page * @pagination[:per_page] < @pagination[:hits]
-  end
-
+  # Calculates the next page number if more results are available.
+  #
+  # @param page [Integer] Current page number
+  # @param hits [Integer] Total number of results available
+  # @return [Integer, nil] Next page number or nil if no more pages
   def next_page(page, hits)
     page + 1 if page * @pagination[:per_page] < hits
   end

test/models/analyzer_test.rb

@@ -8,7 +8,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: hit_count }, :timdex).pagination
+    pagination = Analyzer.new(eq, hit_count, :timdex).pagination
 
     assert pagination.key?(:hits)
     assert pagination.key?(:start)
@@ -28,7 +28,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 2
     }
 
-    pagination = Analyzer.new(eq, { hits: hit_count }, :timdex).pagination
+    pagination = Analyzer.new(eq, hit_count, :timdex).pagination
 
     assert pagination.key?(:hits)
     assert pagination.key?(:start)
@@ -48,7 +48,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 5
     }
 
-    pagination = Analyzer.new(eq, { hits: hit_count }, :timdex).pagination
+    pagination = Analyzer.new(eq, hit_count, :timdex).pagination
 
     assert pagination.key?(:hits)
     assert pagination.key?(:start)
@@ -67,7 +67,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 45 }, :primo).pagination
+    pagination = Analyzer.new(eq, 45, :primo).pagination
 
     assert_equal 45, pagination[:hits]
     assert_equal 1, pagination[:start]
@@ -82,7 +82,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 2
     }
 
-    pagination = Analyzer.new(eq, { hits: 75 }, :timdex).pagination
+    pagination = Analyzer.new(eq, 75, :timdex).pagination
 
     assert_equal 75, pagination[:hits]
     assert_equal 21, pagination[:start]
@@ -97,7 +97,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 0 }, :primo).pagination
+    pagination = Analyzer.new(eq, 0, :primo).pagination
 
     assert_equal 0, pagination[:hits]
     assert_equal 1, pagination[:start]
@@ -112,7 +112,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 68_644_281 }, :primo).pagination
+    pagination = Analyzer.new(eq, 68_644_281, :primo).pagination
 
     # Should show the actual hit count from the API response
     assert_equal 68_644_281, pagination[:hits]
@@ -128,7 +128,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 68_644_281 }, :timdex).pagination
+    pagination = Analyzer.new(eq, 68_644_281, :timdex).pagination
 
     # Should show the actual hit count from the API response
     assert_equal 68_644_281, pagination[:hits]
@@ -144,7 +144,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 0 }, :unknown_source).pagination
+    pagination = Analyzer.new(eq, 0, :unknown_source).pagination
 
     # Should default to 0 hits for unknown source types
     assert_equal 0, pagination[:hits]
@@ -160,7 +160,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 250 }, :all, { hits: 150 }).pagination
+    pagination = Analyzer.new(eq, 250, :all, 150).pagination
 
     # Should combine hits from both APIs: 150 + 250 = 400
     assert_equal 400, pagination[:hits]
@@ -191,7 +191,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 2
     }
 
-    pagination = Analyzer.new(eq, { hits: 500 }, :all, { hits: 300 }).pagination
+    pagination = Analyzer.new(eq, 500, :all, 300).pagination
 
     # Should combine hits: 300 + 500 = 800
     # Page 2 should show results 21-40 of 800
@@ -208,7 +208,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 5 }, :all, { hits: 10_000 }).pagination
+    pagination = Analyzer.new(eq, 5, :all, 10_000).pagination
 
     # Should still combine hits and calculate pagination as expected
     assert_equal 10_005, pagination[:hits]
@@ -218,13 +218,13 @@ class AnalyzerTest < ActiveSupport::TestCase
     refute pagination.key?(:prev)
   end
 
-  test 'analyzer handles one API returning zero results for all tab' do
+  test 'analyzer handles second API returning zero results for all tab' do
     eq = {
       q: 'data',
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 150 }, :all, { hits: 0 }).pagination
+    pagination = Analyzer.new(eq, 150, :all, 0).pagination
 
     assert_equal 150, pagination[:hits]
     assert_equal 1, pagination[:start]
@@ -233,33 +233,33 @@ class AnalyzerTest < ActiveSupport::TestCase
     refute pagination.key?(:prev)
   end
 
-  test 'analyzer handles both APIs returning zero results for all tab' do
+  test 'analyzer handles missing second API response for all tab' do
     eq = {
       q: 'data',
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 0 }, :all, { hits: 0 }).pagination
+    pagination = Analyzer.new(eq, 100, :all).pagination
 
-    assert_equal 0, pagination[:hits]
+    assert_equal 100, pagination[:hits]
     assert_equal 1, pagination[:start]
-    assert_equal 0, pagination[:end]
-    refute pagination.key?(:next)
+    assert_equal 20, pagination[:end]
+    assert_equal 2, pagination[:next]
     refute pagination.key?(:prev)
   end
 
-  test 'analyzer handles missing primo response for all tab' do
+  test 'analyzer handles both APIs returning zero results for all tab' do
     eq = {
       q: 'data',
       page: 1
     }
 
-    pagination = Analyzer.new(eq, { hits: 100 }, :all).pagination
+    pagination = Analyzer.new(eq, 0, :all, 0).pagination
 
-    assert_equal 100, pagination[:hits]
+    assert_equal 0, pagination[:hits]
     assert_equal 1, pagination[:start]
-    assert_equal 20, pagination[:end]
-    assert_equal 2, pagination[:next]
+    assert_equal 0, pagination[:end]
+    refute pagination.key?(:next)
     refute pagination.key?(:prev)
   end
 
@@ -269,7 +269,7 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 500
     }
 
-    pagination = Analyzer.new(eq, { hits: 75_000_000 }, :all, { hits: 25_000_000 }).pagination
+    pagination = Analyzer.new(eq, 75_000_000, :all, 25_000_000).pagination
 
     assert_equal 100_000_000, pagination[:hits]
     assert_equal 9981, pagination[:start] # (500-1) * 20 + 1
@@ -284,20 +284,20 @@ class AnalyzerTest < ActiveSupport::TestCase
       page: 2
     }
 
-    pagination = Analyzer.new(eq, { hits: 100 }, :timdex).pagination
+    pagination = Analyzer.new(eq, 100, :timdex).pagination
     assert_equal 20, pagination[:per_page]
     assert_equal 21, pagination[:start] # (2-1) * 20 + 1
     assert_equal 40, pagination[:end] # 2 * 20
 
     ClimateControl.modify RESULTS_PER_PAGE: '10' do
-      pagination = Analyzer.new(eq, { hits: 100 }, :timdex).pagination
+      pagination = Analyzer.new(eq, 100, :timdex).pagination
       assert_equal 10, pagination[:per_page]
       assert_equal 11, pagination[:start] # (2-1) * 10 + 1
       assert_equal 20, pagination[:end] # 2 * 10
     end
 
     ClimateControl.modify RESULTS_PER_PAGE: '50' do
-      pagination = Analyzer.new(eq, { hits: 200 }, :timdex).pagination
+      pagination = Analyzer.new(eq, 200, :timdex).pagination
       assert_equal 50, pagination[:per_page]
       assert_equal 51, pagination[:start] # (2-1) * 50 + 1
       assert_equal 100, pagination[:end] # 2 * 50