diff --git a/.env.test b/.env.test index 93bf405e..7e6a0a2e 100644 --- a/.env.test +++ b/.env.test @@ -18,6 +18,9 @@ RACK_ATTACK_VERBOSE_LOGGING=false REQUESTS_PER_PERIOD=10 RESULTS_GLOBAL_LIMIT_PER_SEC=10 RESULTS_PER_PAGE=20 +ALL_TAB_RESULTS_PER_SOURCE=50 +ALL_TAB_SCORER=zscore +LOAD_MORE_MAX_RESULTS=200 SYNDETICS_PRIMO_URL=https://syndetics.com/index.php?client=primo TACOS_HOST=FAKE_TACOS_HOST TACOS_URL=http://FAKE_TACOS_HOST/graphql diff --git a/AGENTS.md b/AGENTS.md index 1fa396d7..9be3732c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ This file highlights the important, discoverable conventions and workflows an AI Essential ENV vars for core functionality: `TIMDEX_GRAPHQL`, `PRIMO_API_URL`, `PRIMO_API_KEY`, `RESULTS_PER_PAGE`, `TIMDEX_INDEX`, `TIMDEX_SOURCES`. Filter customization: `FILTER_*` (e.g., `FILTER_LANGUAGE`, `FILTER_CONTENT_TYPE`) and `ACTIVE_FILTERS` (comma-separated list controlling visibility/order of filters; note that filter aggregation keys in the schema use `*Filter` suffix, e.g., `languageFilter`, `contentTypeFilter`). Tests rely on `.env.test` values for VCR cassette generation and use `ClimateControl` gem to mock feature flags. -- **Parallel fetching & multi-source pagination:** The `all` tab uses `MergedSearchService` (with `MergedSearchPaginator`) to fetch Primo and Timdex concurrently via `Thread.new`, then intelligently merges paginated results. Primo has a practical offset limit (~960 records); when this limit is reached, the UI shows a `show_continuation` flag to indicate search is exhausted. Merged totals are cached for 12 hours. Be careful when refactoring to preserve thread-safety, caching semantics, and offset limit handling. +- **Parallel fetching, reranking & load more:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zipper`). `ALL_TAB_RESULTS_PER_SOURCE` (default: 50) controls how many candidates are fetched from each API when the cached all-tab pool needs to grow; `RESULTS_PER_PAGE` controls the initial visible batch size and each "Load more results" increment. The all tab preserves already-shown result order when reranking an expanded candidate pool, appending only unseen records. Source-specific tabs use the same load-more UI but still rely on their normal API page/offset behavior internally. `LOAD_MORE_MAX_RESULTS` caps visible load-more growth. Be careful when refactoring to preserve thread-safety, cache keys, stable-prefix behavior, and source exhaustion handling. - **JS stack & conventions:** Rails importmap is in use (`importmap-rails`). JavaScript entry is `app/javascript/application.js`. Stimulus controllers live in `app/javascript/controllers` and are imported by `importmap` via `config/importmap.rb`. Key controllers include `content_loader_controller.js` (dynamic content loading) and tab management via `source_tabs.js` (which handles geospatial UI state). Prefer small, focused changes to Stimulus controllers rather than heavy bundler-based rewrites. diff --git a/Gemfile b/Gemfile index 07b42619..86ff2a34 100644 --- a/Gemfile +++ b/Gemfile @@ -20,6 +20,7 @@ gem 'rails', '~> 8.1.0' gem 'rails_cloudflare_turnstile' gem 'rails_semantic_logger' gem 'redis' +gem 'reranker', git: 'https://github.com/MITLibraries/reranker.git', tag: 'v0.1.0' gem 'scout_apm' gem 'sentry-rails' gem 'sentry-ruby' diff --git a/Gemfile.lock b/Gemfile.lock index 1f0192f9..a5d4a2a9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,11 @@ +GIT + remote: https://github.com/MITLibraries/reranker.git + revision: 85b0d19c88c853c76f8de54786b4341ad2e43d4d + tag: v0.1.0 + specs: + reranker (0.1.0) + enumerable-stats (>= 1.0, < 2.0) + GIT remote: https://github.com/mitlibraries/mitlibraries-theme revision: 135390daff870937212f1d9020abb6aea53c87d9 @@ -133,6 +141,7 @@ GEM dotenv (= 3.2.0) railties (>= 6.1) drb (2.2.3) + enumerable-stats (1.4.0) erb (6.0.6) erubi (1.13.1) faraday (2.14.3) @@ -452,6 +461,7 @@ DEPENDENCIES rails_cloudflare_turnstile rails_semantic_logger redis + reranker! rubocop rubocop-rails scout_apm diff --git a/README.md b/README.md index 8c66daf2..50af0630 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,10 @@ may have unexpected consequences if applied to other TIMDEX UI apps. - `BLOCKED_USER_AGENTS` - comma-separated list of user agent strings to hard-block with 403 Forbidden responses (bypasses throttling; much cheaper). Default blocks `Sogou web spider` which was responsible for 76.94k spoofed attack requests from non-Chinese IPs. Example: `"Sogou web spider,BadBot/2.0"` - `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_REQUESTS_PER_PERIOD` -- `RESULTS_PER_PAGE`: The number of results to display per page. Use an even number to avoid peculiarities. Defaults to 20 if unset. +- `ALL_TAB_RESULTS_PER_SOURCE`: Number of candidate results to fetch from each API (Primo and TIMDEX) when the combined all tab needs to grow its cached reranked pool. Defaults to 50 if unset. This is not the number of results shown per click; use `RESULTS_PER_PAGE` for that. Keep below 960 to avoid hitting Primo's offset limit. +- `ALL_TAB_SCORER`: Scorer to use when merging results on the combined all tab. Valid values: `zipper` (default), `zscore`, `simple`, `random`. See the [reranker gem](https://github.com/MITLibraries/reranker) for details. +- `LOAD_MORE_MAX_RESULTS`: Maximum number of results that can be shown through load-more requests for a single results tab. Defaults to 200 if unset. This guards against accidental oversized all-tab reranking requests. +- `RESULTS_PER_PAGE`: The number of results shown in the initial batch and added by each "Load more results" click. For source-specific tabs this also remains the underlying API page size. Defaults to 20 if unset. - `ROBOTS_ENV`: Determines which version of `robots.txt` is used. This is read by the Robots controller. Any value other than `production` results in the non-production version being used. - `SCOUT_AUTO_INSTRUMENTS`: default is `false`. Recommended setting is `true` unless we add manual instrumentation in the future. - `SCOUT_DEV_TRACE`: only set in dev. Enables local, in-browser traces (does not send to ScoutAPM service. Other SCOUT ENV should not be set when using this) diff --git a/app/assets/stylesheets/partials/_pagination.scss b/app/assets/stylesheets/partials/_pagination.scss index cd16e5ac..e073b5c2 100644 --- a/app/assets/stylesheets/partials/_pagination.scss +++ b/app/assets/stylesheets/partials/_pagination.scss @@ -54,4 +54,13 @@ .center-elements {display: none;} } -} \ No newline at end of file +} + +.load-more { + clear: both; + display: flex; + justify-content: center; + padding-top: 1em; + border-top: 1px solid $color-border-default; + margin-bottom: 48px; +} diff --git a/app/assets/stylesheets/partials/_results.scss b/app/assets/stylesheets/partials/_results.scss index 79170a11..68eba5d9 100644 --- a/app/assets/stylesheets/partials/_results.scss +++ b/app/assets/stylesheets/partials/_results.scss @@ -45,8 +45,10 @@ } .results-list.use { - list-style-type: none; + list-style-type: decimal; + list-style-position: outside; padding: 0; + padding-left: 2.5rem; margin-bottom: 0; } diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 16e4dd5e..b154fb52 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -25,7 +25,10 @@ def results # Load GeoData results if applicable if Feature.enabled?(:geodata) load_geodata_results - render 'results_geo' + respond_to do |format| + format.turbo_stream { render :results_geo } + format.html { render 'results_geo' } + end return end @@ -42,6 +45,7 @@ def results # Render the response in HTML or JSON format respond_to do |format| format.json { render json: { results: @results, pagination: @pagination, errors: @errors } } + format.turbo_stream { render :results } format.html { render :results } end end @@ -81,6 +85,8 @@ def load_geodata_results raw_results = extract_results(response) @results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize @filters = extract_filters(response) + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_primo_results @@ -89,6 +95,8 @@ def load_primo_results @pagination = data[:pagination] @errors = data[:errors] @show_primo_continuation = data[:show_continuation] + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_timdex_results @@ -96,28 +104,59 @@ def load_timdex_results @results = data[:results] @pagination = data[:pagination] @errors = data[:errors] + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_all_results - current_page = @enhanced_query[:page] || 1 - per_page = ENV.fetch('RESULTS_PER_PAGE', '20').to_i - - # Inject wrapper fetchers instead of using the service defaults. We use lambdas here so the - # service can call back into this controller's instance methods while preserving request-scoped - # context (for example `@enhanced_query`) and the controller's caching/normalization behavior. - # Using lambdas keeps the service decoupled from controller internals. + # Inject wrapper fetchers so the service can call back into this controller's + # instance methods while preserving request-scoped context and caching behavior. merge_service = MergedSearchService.new( enhanced_query: @enhanced_query, active_tab: @active_tab, primo_fetcher: ->(offset:, per_page:, query: nil) { fetch_primo_data(offset: offset, per_page: per_page) }, timdex_fetcher: ->(offset:, per_page:, query: nil) { fetch_timdex_data(offset: offset, per_page: per_page) } ) - data = merge_service.fetch(page: current_page, per_page: per_page) + display_count = sanitized_load_count + stable_count = [display_count - load_more_batch_size, 0].max + data = merge_service.fetch(display_count: display_count, stable_count: stable_count) @results = data[:results] + @append_results = data[:append_results] @errors = data[:errors] - @pagination = data[:pagination] - @show_primo_continuation = data[:show_primo_continuation] + @load_more = data[:load_more] + end + + # Builds the shared load-more view model for source-specific tabs that still + # use traditional page/offset mechanics internally. The UI presents one + # growing list, while the next request simply fetches the next existing page. + def load_more_from_pagination(pagination) + return nil if pagination.blank? + + { + display_count: pagination[:end], + next_page: pagination[:next], + has_more: pagination[:next].present?, + total_hits: pagination[:hits] + } + end + + # Returns the visible all-tab result count requested by the browser. This is + # intentionally separate from `page`: source-specific tabs keep using page for + # API offsets, while the all tab uses a display count to grow a stable reranked + # result list. + def sanitized_load_count + requested = params[:load_count].to_i + requested = load_more_batch_size if requested < 1 + [requested, max_load_count].min + end + + def load_more_batch_size + ENV.fetch('RESULTS_PER_PAGE', '20').to_i + end + + def max_load_count + ENV.fetch('LOAD_MORE_MAX_RESULTS', '200').to_i end def fetch_primo_data(offset: nil, per_page: nil) diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index 9aa6655c..4845ab78 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -14,7 +14,7 @@ function swapTabs(new_target) { } } -// Loading spinner behavior for pagination (Turbo Frame updates) +// Loading spinner behavior for frame and stream updates document.addEventListener('turbo:frame-render', function(event) { if (window.pendingFocusAction === 'pagination') { // Focus on first result for pagination @@ -51,13 +51,29 @@ document.addEventListener('turbo:frame-render', function(event) { }; }); +document.addEventListener('turbo:before-stream-render', function(event) { + if (window.pendingFocusAction === 'load-more') { + document.getElementById('search-results')?.classList.remove('spinner'); + window.pendingFocusAction = null; + } +}); + document.addEventListener('click', function(event) { const clickedElement = event.target; + const loadMoreLink = clickedElement.closest('.load-more-link'); + + // Handle load-more clicks. Results append in place, so do not scroll users + // back to the top or move focus away from their current reading position. + if (loadMoreLink) { + document.getElementById('search-results')?.classList.add('spinner'); + window.pendingFocusAction = 'load-more'; + return; + } // Handle pagination clicks if (clickedElement.matches('.first a, .previous a, .next a')) { // Throw the spinner on the search results immediately - document.getElementById('search-results').classList.add('spinner'); + document.getElementById('search-results')?.classList.add('spinner'); // Position the window at the top of the results window.scrollTo({ top: 0, behavior: 'smooth' }); @@ -100,4 +116,3 @@ document.addEventListener('turbo:load', function(event) { } }); - diff --git a/app/models/merged_search_paginator.rb b/app/models/merged_search_paginator.rb deleted file mode 100644 index 19f36934..00000000 --- a/app/models/merged_search_paginator.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -# MergedSearchPaginator encapsulates stateless merged pagination logic for combining two API result sets. -# It calculates the merge plan, API offsets, and merges the results for a given page. -class MergedSearchPaginator - attr_reader :primo_total, :timdex_total, :current_page, :per_page - - def initialize(primo_total:, timdex_total:, current_page:, per_page:) - @primo_total = primo_total - @timdex_total = timdex_total - @current_page = current_page - @per_page = per_page - end - - # Returns an array of :primo and :timdex symbols for the merged result order on this page - def merge_plan - total_results = primo_total + timdex_total - start_index = (current_page - 1) * per_page - end_index = [start_index + per_page, total_results].min - plan = [] - primo_used = 0 - timdex_used = 0 - i = 0 - while i < end_index - if primo_used < primo_total && (timdex_used >= timdex_total || primo_used <= timdex_used) - source = :primo - primo_used += 1 - elsif timdex_used < timdex_total - source = :timdex - timdex_used += 1 - end - plan << source if i >= start_index - i += 1 - end - plan - end - - # Returns [primo_offset, timdex_offset] for the start of this page - # Returns [primo_offset, timdex_offset] for the start of this page. - # - # If an API has been exhausted (the computed offset is greater-than-or-equal - # to that API's total), this method returns `nil` for that API to signal - # callers that no request should be made against that service for this page. - # - # This avoids unnecessary requests where the API would only return empty - # results when queried at an offset beyond its available records. - def api_offsets - start_index = (current_page - 1) * per_page - primo_offset = 0 - timdex_offset = 0 - i = 0 - while i < start_index - if primo_offset < primo_total && (timdex_offset >= timdex_total || primo_offset <= timdex_offset) - primo_offset += 1 - elsif timdex_offset < timdex_total - timdex_offset += 1 - else - break - end - i += 1 - end - # If the computed offset reached or exceeded the total for a source, - # return nil for that source to indicate it is exhausted. - primo_offset = nil if primo_offset >= primo_total - timdex_offset = nil if timdex_offset >= timdex_total - - [primo_offset, timdex_offset] - end -end diff --git a/app/models/merged_search_service.rb b/app/models/merged_search_service.rb index 880c0b7d..d86cae17 100644 --- a/app/models/merged_search_service.rb +++ b/app/models/merged_search_service.rb @@ -1,20 +1,17 @@ -require 'digest' - -# Orchestrates merged "all" tab searches across Primo and TIMDEX. +# Orchestrates merged "all" tab searches across Primo and TIMDEX using the +# reranker gem. # -# Handles parallel fetches, per-query totals caching, pagination calculation via -# `MergedSearchPaginator`, and assembly of a controller-friendly response hash. +# The all tab cannot use traditional page numbers because each "load more" +# request may add new candidates from both source APIs and reranking that larger +# pool can change global order. To keep the user experience stable, this service +# caches a per-query candidate pool and ordered list of record keys. When the +# pool grows, it reranks everything again but preserves the prefix the browser +# has already displayed, appending only unseen records after that prefix. class MergedSearchService - # Time to live value for cache expiration. TTL = 12.hours # Initialize a new MergedSearchService. # - # The service requires two callable fetchers (for Primo and TIMDEX) that - # perform the underlying source requests. Fetchers are injected to keep - # the orchestration logic decoupled from transport, caching, and - # normalization concerns. - # # @param enhanced_query [Hash] query hash produced by `Enhancer` # @param active_tab [String] the currently active tab (e.g. 'all') # @param primo_fetcher [#call] callable used to fetch Primo results; must accept `offset:, per_page:, query:` @@ -26,234 +23,250 @@ def initialize(enhanced_query:, active_tab:, primo_fetcher:, timdex_fetcher:) @timdex_fetcher = timdex_fetcher end - # Execute merged search orchestration for the requested page. + # Fetch and rerank results from both sources for the all tab. # - # @param page [Integer] page number to fetch - # @param per_page [Integer] number of results per page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def fetch(page:, per_page:) - current_page = (page || 1).to_i - per_page = (per_page || 20).to_i - if current_page == 1 - first_page_fetch(current_page, per_page) - else - deeper_page_fetch(current_page, per_page) - end - end + # @param display_count [Integer, nil] number of ordered results the caller + # wants visible after this request. + # @param stable_count [Integer] number of leading results already displayed in + # the browser. Those records retain their relative order even if the expanded + # candidate pool would rerank them differently. + # @param per_source [Integer, nil] number of results to request from each API + # per source fetch; defaults to ALL_TAB_RESULTS_PER_SOURCE or 50. + # @return [Hash] keys: :results, :append_results, :errors, :load_more + def fetch(display_count: nil, stable_count: 0, per_source: nil) + per_source = (per_source || ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50')).to_i + display_count = (display_count || ENV.fetch('RESULTS_PER_PAGE', '20')).to_i + display_count = [display_count, 1].max + stable_count = [stable_count.to_i, 0].max - # Handle page 1: perform the full-size parallel fetch, cache - # totals, build the paginator, and return the assembled result. - # - # Executes a full-size parallel fetch (requests `per_page` items from each - # backend), computes and caches per-query totals, constructs a - # `MergedSearchPaginator`, and assembles the controller-facing response. - # - # @param current_page [Integer] the current page (expected to be 1) - # @param per_page [Integer] the number of results per merged page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def first_page_fetch(current_page, per_page) - primo_data, timdex_data = parallel_fetch(offset: 0, per_page: per_page) + state = Rails.cache.read(state_cache_key) || empty_state + state = ensure_ordered_results(state, display_count: display_count, stable_count: stable_count, + per_source: per_source) + Rails.cache.write(state_cache_key, state, expires_in: TTL) - totals = { primo: primo_data[:hits].to_i, timdex: timdex_data[:hits].to_i } - write_cached_totals(totals) + ordered_results = records_for_keys(state[:ordered_keys], state).first(display_count) + append_results = ordered_results[stable_count...display_count] || [] + total_hits = state[:primo_hits].to_i + state[:timdex_hits].to_i + + { + results: ordered_results, + append_results: append_results, + errors: state[:errors], + load_more: { + display_count: display_count, + next_count: display_count + visible_batch_size, + has_more: has_more?(state, display_count), + total_hits: total_hits + } + } + end - paginator = build_paginator_from_totals(totals, current_page, per_page) + private - assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page) + # Returns an empty serializable cache state for a merged all-tab search. + # Source result arrays hold normalized records. `ordered_keys` is the stable + # display order, represented by deterministic record keys so duplicated + # records can be removed safely when additional source chunks arrive. + def empty_state + { + primo_results: [], + timdex_results: [], + ordered_keys: [], + primo_hits: 0, + timdex_hits: 0, + primo_exhausted: false, + timdex_exhausted: false, + errors: nil + } end - # Handle deeper pages: ensure totals are available (falling back to summary - # calls when missing), build the paginator, fetch required chunks, and - # assemble the final result. - # - # Ensures per-query totals are available by reading cached totals or - # performing summary requests (per_page == 1) if the cached totals aren't - # present. (They should be, but there may be edge cases.) - # Builds a `MergedSearchPaginator`, fetches the page-sized chunks required - # for the merged layout, and returns the assembled controller-facing response. - # - # @param current_page [Integer] the requested page number (> 1) - # @param per_page [Integer] the number of results per merged page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def deeper_page_fetch(current_page, per_page) - totals = Rails.cache.read(totals_cache_key) - - unless totals - primo_summary, timdex_summary = parallel_fetch(offset: 0, per_page: 1) - totals = { primo: primo_summary[:hits].to_i, timdex: timdex_summary[:hits].to_i } - write_cached_totals(totals) + # Ensures the cached state contains enough ordered results to satisfy the + # requested display count, fetching more source candidates when necessary. + def ensure_ordered_results(state, display_count:, stable_count:, per_source:) + state = rerank_state(state, stable_count: stable_count) if state[:ordered_keys].empty? && any_results?(state) + + while state[:ordered_keys].length < display_count && sources_available?(state) + previous_count = state[:ordered_keys].length + state = fetch_next_source_chunks(state, per_source: per_source) + state = rerank_state(state, stable_count: stable_count) + break if state[:ordered_keys].length == previous_count end - paginator = build_paginator_from_totals(totals, current_page, per_page) - primo_data, timdex_data = fetch_all_tab_page_chunks(paginator) + state + end + + # Fetches the next chunk from each source that can still produce records. The + # source offset is simply the number of normalized records already cached for + # that source, which preserves each API's native offset semantics while the + # service manages display order separately. + def fetch_next_source_chunks(state, per_source:) + primo_offset = state[:primo_results].length + timdex_offset = state[:timdex_results].length + + primo_data, timdex_data = parallel_fetch( + primo_offset: state[:primo_exhausted] || primo_offset >= Analyzer::PRIMO_MAX_OFFSET ? nil : primo_offset, + timdex_offset: state[:timdex_exhausted] ? nil : timdex_offset, + per_page: per_source + ) + + update_state_from_source!(state, :primo, primo_data, requested_offset: primo_offset, per_page: per_source) + update_state_from_source!(state, :timdex, timdex_data, requested_offset: timdex_offset, per_page: per_source) + state[:errors] = combine_errors(state[:errors], primo_data&.[](:errors), timdex_data&.[](:errors)) - assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page, deeper: true) + state end - private + # Merges a source response into the cached state and records whether that + # source appears exhausted. A source is exhausted when it returns fewer records + # than requested, reports no more total hits, or Primo signals continuation. + def update_state_from_source!(state, source, data, requested_offset:, per_page:) + return mark_exhausted!(state, source, reason: 'fetch skipped') if data.nil? - # Generate the cache key used to store per-query totals for this enhanced query/tab. - # - # @return [String] cache key ending in '/totals' - def totals_cache_key - base = generate_cache_key(@enhanced_query.merge(tab: @active_tab)) - "#{base}/totals" + results_key = source_results_key(source) + hits_key = source_hits_key(source) + incoming = Array(data[:results]) + + state[hits_key] = data[:hits].to_i + state[results_key] = dedupe_records(state[results_key] + incoming) + + exhaustion_reason = exhaustion_reason(data, incoming, requested_offset, per_page, state[hits_key]) + return unless exhaustion_reason + + mark_exhausted!(state, source, reason: exhaustion_reason, offset: requested_offset, + returned: incoming.length, hits: state[hits_key]) end - # Persist per-query totals to the application cache. - # - # Tests use a test-local `Rails.cache` (MemoryStore) so they do not need to - # inject a separate cache instance; production code uses the configured - # `Rails.cache` store. - # - # @param totals [Hash] { primo: Integer, timdex: Integer } - def write_cached_totals(totals) - Rails.cache.write(totals_cache_key, totals, expires_in: TTL) + def mark_exhausted!(state, source, reason:, offset: nil, returned: nil, hits: nil) + state[:"#{source}_exhausted"] = true + Rails.logger.debug do + details = { source: source, reason: reason, offset: offset, returned: returned, hits: hits }.compact + "All-tab load more exhausted source: #{details}" + end end - # Perform parallel fetches from Primo and TIMDEX using the configured - # fetchers. Each fetcher should return the usual response hash including - # `:results` and `:hits`. + # Returns a short reason string when a source can no longer provide useful + # load-more candidates, or nil while the source should remain available. + def exhaustion_reason(data, incoming, requested_offset, per_page, hits) + return 'Primo continuation required' if data[:show_continuation] + return 'empty response' if incoming.empty? + return 'short response' if incoming.length < per_page + return 'reported hits reached' if hits.to_i.positive? && requested_offset + incoming.length >= hits.to_i + + nil + end + + # Fetch from Primo and TIMDEX concurrently. # - # WARNING: exceptions raised inside these threads will not automatically - # propagate to the caller; callers/tests should account for this. + # WARNING: exceptions raised inside threads do not automatically propagate; + # callers should account for this. # - # @param offset [Integer] api offset to request - # @param per_page [Integer] number of items to request - # @return [Array] [primo_response, timdex_response] - def parallel_fetch(offset:, per_page:) + # @param primo_offset [Integer, nil] next Primo offset, or nil to skip Primo + # @param timdex_offset [Integer, nil] next TIMDEX offset, or nil to skip TIMDEX + # @param per_page [Integer] number of results to request per source + # @return [Array] [primo_response, timdex_response] + def parallel_fetch(primo_offset:, timdex_offset:, per_page:) primo = nil timdex = nil threads = [] - threads << Thread.new { primo = @primo_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } - threads << Thread.new { timdex = @timdex_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } + if primo_offset && primo_offset < Analyzer::PRIMO_MAX_OFFSET + threads << Thread.new do + primo = @primo_fetcher.call(offset: primo_offset, per_page: per_page, query: @enhanced_query) + end + end + if timdex_offset + threads << Thread.new do + timdex = @timdex_fetcher.call(offset: timdex_offset, per_page: per_page, query: @enhanced_query) + end + end threads.each(&:join) [primo, timdex] end - # Compute API offsets from the paginator and fetch the page-sized chunks - # required to assemble the merged page. - # - # @param paginator [MergedSearchPaginator] - # @return [Array] [primo_data, timdex_data] - def fetch_all_tab_page_chunks(paginator) - merge_plan = paginator.merge_plan - primo_count = merge_plan.count(:primo) - timdex_count = merge_plan.count(:timdex) - primo_offset, timdex_offset = paginator.api_offsets - - # Only spawn fetch threads when we both need results for the merge plan - # and the paginator indicates a valid offset for that API. A `nil` offset - # means the API is exhausted and should not be queried for this page. - primo_thread = if primo_count > 0 && !primo_offset.nil? - Thread.new do - @primo_fetcher.call(offset: primo_offset, per_page: primo_count, query: @enhanced_query) - end - end - timdex_thread = if timdex_count > 0 && !timdex_offset.nil? - Thread.new do - @timdex_fetcher.call(offset: timdex_offset, per_page: timdex_count, query: @enhanced_query) - end - end - - primo_data = if primo_thread - primo_thread.value - else - { results: [], errors: nil, hits: paginator.primo_total, - show_continuation: false } - end - timdex_data = timdex_thread ? timdex_thread.value : { results: [], errors: nil, hits: paginator.timdex_total } - - [primo_data, timdex_data] + # Reranks the expanded candidate pool and preserves any already-visible prefix. + # This method intentionally treats the reranker as an interchangeable ordering + # engine: any scorer that returns ordered result hashes through the gem's + # stable API can be used without changing the load-more logic. + def rerank_state(state, stable_count:) + prefix = state[:ordered_keys].first(stable_count) + reranked = Reranker::Reranker.new(configured_scorer).rerank( + state[:primo_results], state[:timdex_results], query: @enhanced_query[:q] + ) + reranked_keys = dedupe_keys(reranked.map { |record| record_key(record) }) + + state[:ordered_keys] = prefix + reranked_keys.reject { |key| prefix.include?(key) } + state end - # Assemble the final hash returned to the controller for rendering. + # Instantiate the scorer based on the ALL_TAB_SCORER env var. + # Defaults to ZipperMergeScorer. # - # @param paginator [MergedSearchPaginator] - # @param primo_data [Hash] response from Primo fetcher - # @param timdex_data [Hash] response from TIMDEX fetcher - # @param current_page [Integer] - # @param per_page [Integer] - # @param deeper [Boolean] whether this was a deeper-page flow - # @return [Hash] response with :results, :errors, :pagination, :show_primo_continuation - def assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page, deeper: false) - primo_total = primo_data[:hits] || 0 - timdex_total = timdex_data[:hits] || 0 - - merged = merge_results(paginator, primo_data[:results] || [], timdex_data[:results] || []) - errors = combine_errors(primo_data[:errors], timdex_data[:errors]) - pagination = Analyzer.new(@enhanced_query, timdex_total, :all, primo_total, per_page).pagination - - show_primo_continuation = if deeper - # Use the Primo-specific API offset (calculated from the paginator) - # when deciding whether to show a Primo continuation. - # - # If the paginator returns `nil` for a exhausted API we still - # want to show the continuation when the requested page is far - # beyond the Primo API's practical offset limit. Fall back to - # checking the merged page's start index when the API offset - # is unavailable. - primo_api_offset, _timdex_api_offset = paginator.api_offsets - primo_data[:show_continuation] || - (primo_api_offset && primo_api_offset >= Analyzer::PRIMO_MAX_OFFSET) || - (primo_api_offset.nil? && ((current_page - 1) * per_page) >= Analyzer::PRIMO_MAX_OFFSET) - else - primo_data[:show_continuation] - end - - { results: merged, errors: errors, pagination: pagination, show_primo_continuation: show_primo_continuation } + # @return [Reranker::Scorer] + def configured_scorer + case ENV.fetch('ALL_TAB_SCORER', 'zipper').downcase + when 'zscore' then Reranker::ZscoreScorer.new + when 'zipper' then Reranker::ZipperMergeScorer.new + when 'simple' then Reranker::SimpleScorer.new + when 'random' then Reranker::RandomScorer.new + else Reranker::ZscoreScorer.new + end end - # Merge multiple error arrays into a single array or nil when empty. - # - # @return [Array, nil] - def combine_errors(*error_arrays) - all_errors = error_arrays.compact.flatten - all_errors.any? ? all_errors : nil + def state_cache_key + query = @enhanced_query.except(:page).merge( + tab: @active_tab, + scorer: ENV.fetch('ALL_TAB_SCORER', 'zipper'), + boost_sources: ENV.fetch('ALL_TAB_BOOST_SOURCES', ''), + per_source: ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50') + ) + "#{CacheKeyGenerator.call(query)}/all-tab-load-more" end - # Build a `MergedSearchPaginator` given cached totals. - # - # @param totals [Hash] { primo: Integer, timdex: Integer } - # @return [MergedSearchPaginator] - def build_paginator_from_totals(totals, current_page, per_page) - MergedSearchPaginator.new(primo_total: totals[:primo] || 0, timdex_total: totals[:timdex] || 0, - current_page: current_page, per_page: per_page) + def visible_batch_size + ENV.fetch('RESULTS_PER_PAGE', '20').to_i end - # Note: default fetcher implementations were removed to enforce explicit - # dependency injection. Callers must provide `primo_fetcher` and - # `timdex_fetcher` when constructing `MergedSearchService`. + def any_results?(state) + state[:primo_results].any? || state[:timdex_results].any? + end - # Generate a cache key based on the supplied query hash. - # - # @param query [Hash] - # @return [String] MD5 hex digest - def generate_cache_key(query) - CacheKeyGenerator.call(query) + def sources_available?(state) + !state[:primo_exhausted] || !state[:timdex_exhausted] + end + + def has_more?(state, display_count) + state[:ordered_keys].length > display_count || sources_available?(state) + end + + def source_results_key(source) + :"#{source}_results" end - # Helps callers (including `MergedSearchPaginator`) delegate merging logic to the orchestration - # layer. This method iterates the paginator's `merge_plan` and pulls items from the respective - # source result arrays in order. + def source_hits_key(source) + :"#{source}_hits" + end + + def records_for_keys(keys, state) + records_by_key = (state[:primo_results] + state[:timdex_results]).index_by { |record| record_key(record) } + keys.filter_map { |key| records_by_key[key] } + end + + def dedupe_records(records) + records.reverse.index_by { |record| record_key(record) }.values.reverse + end + + def dedupe_keys(keys) + keys.each_with_object([]) { |key, unique| unique << key unless unique.include?(key) } + end + + def record_key(record) + [record[:api], record[:identifier] || record[:sourceLink] || record[:source_link] || record[:title]].join(':') + end + + # Merge multiple error arrays into a single array, or nil when empty. # - # @param paginator [MergedSearchPaginator] - # @param primo_results [Array] - # @param timdex_results [Array] - # @return [Array] merged results - def merge_results(paginator, primo_results, timdex_results) - merged = [] - primo_idx = 0 - timdex_idx = 0 - paginator.merge_plan.each do |source| - if source == :primo - merged << primo_results[primo_idx] if primo_idx < primo_results.length - primo_idx += 1 - else - merged << timdex_results[timdex_idx] if timdex_idx < timdex_results.length - timdex_idx += 1 - end - end - merged + # @return [Array, nil] + def combine_errors(*error_arrays) + all_errors = error_arrays.compact.flatten + all_errors.any? ? all_errors : nil end end diff --git a/app/views/search/_load_more.html.erb b/app/views/search/_load_more.html.erb new file mode 100644 index 00000000..545d4b9c --- /dev/null +++ b/app/views/search/_load_more.html.erb @@ -0,0 +1,22 @@ +<%# Shared load-more control. The UI is consistent across tabs, while the + backing params differ: all tab grows by visible result count; source tabs + keep using page internally to request the next API offset. %> +<% if @load_more&.[](:has_more) %> + <% params_copy = @enhanced_query.dup %> + <% params_copy[:tab] = @active_tab if @active_tab.present? %> + <% if @active_tab == 'all' %> + <% params_copy.delete(:page) %> + <% params_copy[:load_count] = @load_more[:next_count] %> + <% else %> + <% params_copy[:page] = @load_more[:next_page] %> + <% end %> + +
+ <%= link_to results_path(params_copy), class: 'btn button-secondary load-more-link', + data: { turbo_stream: true }, rel: 'nofollow' do %> + Load more results + <% end %> +
+<% else %> +
+<% end %> diff --git a/app/views/search/_result_item.html.erb b/app/views/search/_result_item.html.erb new file mode 100644 index 00000000..7b21067f --- /dev/null +++ b/app/views/search/_result_item.html.erb @@ -0,0 +1,8 @@ +<%# Renders one normalized result regardless of source API. Keeping this branch in + one partial lets full-page HTML and Turbo Stream append responses use exactly + the same result markup. %> +<% if result[:api] == 'primo' %> + <%= render(partial: 'search/result_primo', locals: { result: result }) %> +<% elsif result[:api] == 'timdex' %> + <%= render(partial: 'search/result', locals: { result: result }) %> +<% end %> diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index f5cf8bbe..a3d67290 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -17,23 +17,17 @@ <% elsif @results.present? && @errors.blank? %>
-

<%= results_summary(@pagination[:hits]) %>

+

<%= results_summary(@pagination&.[](:hits) || @load_more&.[](:total_hits) || @results.length) %>

<% if @show_nls_warning %> <%= render partial: 'search/nls_alert' %> <% end %>
-
    - <% @results.each do |result| %> - <% if result[:api] == 'primo' %> - <%= render(partial: 'search/result_primo', locals: { result: result }) %> - <% elsif result[:api] == 'timdex' %> - <%= render(partial: 'search/result', locals: { result: result }) %> - <% end %> - <% end %> +
      + <%= render partial: 'search/result_item', collection: @results, as: :result %>
    - <%= render partial: "pagination" %> + <%= render partial: "load_more" %> <%# Note `results_callouts` is also displayed in no errors condition below %> <%= render partial: "results_callouts" %>
diff --git a/app/views/search/results.turbo_stream.erb b/app/views/search/results.turbo_stream.erb new file mode 100644 index 00000000..ebf5dd54 --- /dev/null +++ b/app/views/search/results.turbo_stream.erb @@ -0,0 +1,7 @@ +<%= turbo_stream.append 'results-list' do %> + <%= render partial: 'search/result_item', collection: @append_results, as: :result %> +<% end %> + +<%= turbo_stream.replace 'load-more' do %> + <%= render partial: 'search/load_more' %> +<% end %> diff --git a/app/views/search/results_geo.html.erb b/app/views/search/results_geo.html.erb index 1d025023..78d371c0 100644 --- a/app/views/search/results_geo.html.erb +++ b/app/views/search/results_geo.html.erb @@ -36,7 +36,7 @@
<% if @results.present? && @errors.blank? %>

<%= results_summary(@pagination[:hits]) %> returned

-
    +
      <%= render(partial: 'search/result_geo', collection: @results, as: :result_geo) %>
    <% elsif @errors.blank? %> @@ -50,9 +50,7 @@ <%= render partial: 'shared/ask', locals: { display: 'aside' } if @results.blank? %> <% if @results.present? %> - + <%= render partial: "load_more" %> <%= render partial: 'shared/ask', locals: { display: 'view-md' } %> <% end %>
diff --git a/app/views/search/results_geo.turbo_stream.erb b/app/views/search/results_geo.turbo_stream.erb new file mode 100644 index 00000000..27606b96 --- /dev/null +++ b/app/views/search/results_geo.turbo_stream.erb @@ -0,0 +1,7 @@ +<%= turbo_stream.append 'results-list' do %> + <%= render partial: 'search/result_geo', collection: @append_results, as: :result_geo %> +<% end %> + +<%= turbo_stream.replace 'load-more' do %> + <%= render partial: 'search/load_more' %> +<% end %> diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 010c527e..b8916cd4 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -449,18 +449,18 @@ def build_timdex_mock_response end end - test 'primo results with valid query has div for pagination' do + test 'primo results with valid query has load more control' do mock_primo_search_success get '/results?q=data&tab=primo' assert_response :success - assert_select '#pagination' + assert_select '#load-more' end - test 'timdex results with valid query has div for pagination' do + test 'timdex results with valid query has load more control' do mock_timdex_search_success get '/results?q=data&tab=timdex' assert_response :success - assert_select '#pagination' + assert_select '#load-more' end test 'primo results with valid query has div for results which is populated' do @@ -881,26 +881,12 @@ def source_filter_count(controller) assert_select 'a[href*="tab=website"]', count: 1 end - test 'all tab page 1 writes totals to cache' do - # This integration-level behavior is covered by unit tests on `MergedSearchService`. - # Here we assert the controller delegates to the service. + test 'all tab delegates to MergedSearchService' do mock_service = mock('merged_service') - mock_service.expects(:fetch).returns({ results: [], errors: nil, pagination: {}, show_primo_continuation: false }) + mock_service.expects(:fetch).returns({ results: [], errors: nil }) MergedSearchService.expects(:new).returns(mock_service) - get '/results?q=test' - assert_response :success - end - - test 'all tab deeper page reads cached totals and avoids summary calls' do - # This behavior is covered in greater depth by `MergedSearchService` unit tests. - mock_service = mock('merged_service') - mock_service.expects(:fetch).with(page: 2, - per_page: 20).returns({ results: [], - errors: nil, pagination: {}, show_primo_continuation: false }) - MergedSearchService.expects(:new).returns(mock_service) - - get '/results?q=test&page=2' + get '/results?q=test&tab=all' assert_response :success end @@ -1067,120 +1053,14 @@ def source_filter_count(controller) assert_select '.tab-navigation .tab-link.active', text: 'All' end - test 'all tab shows primo continuation when page exceeds API offset limit' do - sample_doc = { - api: 'primo', - title: 'Sample Primo Document Title', - format: 'Article', - year: '2025', - creators: [ - { value: 'Foo Barston', link: nil }, - { value: 'Baz Quxley', link: nil } - ], - links: [{ 'kind' => 'full record', 'url' => 'https://example.com/record' }] - } - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ 'docs' => [sample_doc], 'info' => { 'total' => 1 } }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns([sample_doc]).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_success - - get '/results?q=test&tab=all&page=49' - assert_response :success - - # Should show primo continuation partial - assert_select '.primo-continuation', count: 1 - assert_select '.primo-continuation h2', text: /You have reached the limit for these search results/ - end - - test 'all tab pagination displays combined hit counts' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) - - get '/results?q=test&tab=all' - assert_response :success - - # Should show pagination with combined hit counts (500 + 300 = 800) - assert_select '.pagination-container' - assert_select '.pagination-container .current', text: /1 - 20 of 800/ - end - - test 'all tab pagination includes next page link when more results available' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) + test 'all tab does not show pagination controls' do + mock_primo_search_all_tab + mock_timdex_search_all_tab get '/results?q=test&tab=all' assert_response :success - - # Should show next page link when there are more than 20 total results - assert_select '.pagination-container .next a[href*="page=2"]' - end - - test 'all tab pagination on page 2 includes previous page link' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) - - get '/results?q=test&tab=all&page=2' - assert_response :success - - # Should show previous page link - assert_select '.pagination-container .previous a[href*="page=1"]' - - # Should show current range (21-40 for page 2) - assert_select '.pagination-container .current', text: /21 - 40 of 800/ + assert_select '#pagination', count: 0 + assert_select '#load-more' end test 'results can be returned in JSON format when env is set and valid token is provided' do diff --git a/test/models/merged_search_paginator_test.rb b/test/models/merged_search_paginator_test.rb deleted file mode 100644 index 948c8c22..00000000 --- a/test/models/merged_search_paginator_test.rb +++ /dev/null @@ -1,62 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class MergedSearchPaginatorTest < ActiveSupport::TestCase - test 'merge_plan handles balanced results' do - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 3, current_page: 1, per_page: 6) - assert_equal(%i[primo timdex primo timdex primo timdex], paginator.merge_plan) - end - - test 'merge_plan handles unbalanced results' do - paginator = MergedSearchPaginator.new(primo_total: 6, timdex_total: 2, current_page: 1, per_page: 8) - assert_equal(%i[primo timdex primo timdex primo primo primo primo], paginator.merge_plan) - end - - test 'api_offsets are calculated as expected' do - paginator = MergedSearchPaginator.new(primo_total: 10, timdex_total: 10, current_page: 2, per_page: 5) - assert_equal([3, 2], paginator.api_offsets) - end - - test 'merge_results handles even results' do - paginator = MergedSearchPaginator.new(primo_total: 2, timdex_total: 2, current_page: 1, per_page: 4) - primo = %w[P1 P2] - timdex = %w[T1 T2] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - assert_equal(%w[P1 T1 P2 T2], svc.send(:merge_results, paginator, primo, timdex)) - end - - test 'merge_results with shorter array' do - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 1, current_page: 1, per_page: 4) - primo = %w[P1 P2 P3] - timdex = %w[T1] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - assert_equal(%w[P1 T1 P2 P3], svc.send(:merge_results, paginator, primo, timdex)) - end - - test 'api_offsets breaks when start_index exceeds totals' do - # Use very small totals and request a page far beyond available results to exercise the break - paginator = MergedSearchPaginator.new(primo_total: 1, timdex_total: 1, current_page: 5, per_page: 20) - primo_offset, timdex_offset = paginator.api_offsets - - # Offsets should stop at the available totals (1 each) - assert_nil primo_offset - assert_nil timdex_offset - end - - test 'merge_plan returns all primo when timdex is empty' do - paginator = MergedSearchPaginator.new(primo_total: 2, timdex_total: 0, current_page: 1, per_page: 5) - plan = paginator.merge_plan - - assert_equal %i[primo primo], plan - end - - test 'merge_plan returns all timdex when primo is empty' do - paginator = MergedSearchPaginator.new(primo_total: 0, timdex_total: 2, current_page: 1, per_page: 5) - plan = paginator.merge_plan - - assert_equal %i[timdex timdex], plan - end -end diff --git a/test/models/merged_search_service_test.rb b/test/models/merged_search_service_test.rb index dffb47c8..db789da9 100644 --- a/test/models/merged_search_service_test.rb +++ b/test/models/merged_search_service_test.rb @@ -1,160 +1,213 @@ require 'test_helper' -require 'ostruct' class MergedSearchServiceTest < ActiveSupport::TestCase - test 'page 1 writes totals to cache' do + test 'fetch writes state cache with source hit totals' do query = { q: 'test' } primo_fetcher = lambda do |offset:, per_page:, query:| - { results: ['foo'], hits: 42, errors: nil, show_continuation: false } + { results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], hits: 42, errors: nil, + show_continuation: false } end timdex_fetcher = lambda do |offset:, per_page:, query:| - { results: ['bar'], hits: 37, errors: nil } + { results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], hits: 37, errors: nil } end service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) - res = service.fetch(page: 1, per_page: 20) + res = service.fetch(display_count: 20) assert_equal 2, res[:results].length # Verify cache written - key = service.send(:totals_cache_key) + key = service.send(:state_cache_key) cached = Rails.cache.read(key) refute_nil cached - assert_equal 42, cached[:primo] - assert_equal 37, cached[:timdex] + assert_equal 42, cached[:primo_hits] + assert_equal 37, cached[:timdex_hits] end - test 'deeper page reads cached totals and avoids summary calls' do + test 'fetch uses cached state when enough ordered results already exist' do query = { q: 'test' } - service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', - primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) - - # populate cache so service uses it instead of summary calls - Rails.cache.write(service.send(:totals_cache_key), { primo: 50, timdex: 50 }) - - # fetchers that would raise if a summary call (per_page == 1) is attempted + # Fetchers that would raise if the service attempted to fetch despite a + # cache hit with enough ordered results. primo_fetcher = lambda do |offset:, per_page:, query:| - raise 'Summary call made' if per_page == 1 - - { results: ['foo'], hits: 50, errors: nil, show_continuation: false } + raise 'Unexpected Primo fetch' end timdex_fetcher = lambda do |offset:, per_page:, query:| - raise 'Summary call made' if per_page == 1 - - { results: ['bar'], hits: 50, errors: nil } + raise 'Unexpected TIMDEX fetch' end service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + cached_state = { + primo_results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], + timdex_results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], + ordered_keys: %w[primo:p1 timdex:t1], + primo_hits: 50, + timdex_hits: 50, + primo_exhausted: false, + timdex_exhausted: false, + errors: nil + } + Rails.cache.write(service.send(:state_cache_key), cached_state) + # Should not raise assert_nothing_raised do - res = service.fetch(page: 2, per_page: 20) + res = service.fetch(display_count: 2, stable_count: 0) assert res[:results].is_a?(Array) end end - test 'falls back to summary and writes cache when totals are missing' do + test 'fetch calls both fetchers at offset 0 with configured per_source' do q = { q: 'test' } calls = [] - primo_fetcher = lambda do |offset:, per_page:, query:| - calls << [:primo, offset, per_page] - if per_page == 1 - { results: [], hits: 7, errors: nil, show_continuation: false } - else - { results: ['foo'], hits: 7, errors: nil, show_continuation: false } - end - end - - timdex_fetcher = lambda do |offset:, per_page:, query:| - calls << [:timdex, offset, per_page] - if per_page == 1 - { results: [], hits: 3, errors: nil } - else - { results: ['bar'], hits: 3, errors: nil } - end + fetcher = lambda do |offset:, per_page:, query: nil| + calls << [offset, per_page] + { results: [], hits: 0, errors: nil, show_continuation: false } end - svc = MergedSearchService.new(enhanced_query: q, active_tab: 'all', primo_fetcher: primo_fetcher, - timdex_fetcher: timdex_fetcher) + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fetcher, timdex_fetcher: fetcher) + ClimateControl.modify(ALL_TAB_RESULTS_PER_SOURCE: '25') { svc.fetch } - res = svc.fetch(page: 2, per_page: 20) + assert_equal 2, calls.length + calls.each do |offset, per_page| + assert_equal 0, offset + assert_equal 25, per_page + end + end - # summary calls should have been made with per_page == 1 - assert_includes calls, [:primo, 0, 1] - assert_includes calls, [:timdex, 0, 1] + test 'fetch returns merged results from both sources' do + primo_fetcher = fake_fetcher(results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], hits: 1) + timdex_fetcher = fake_fetcher(results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], hits: 1) - # totals cached - key = svc.send(:totals_cache_key) - totals = Rails.cache.read(key) - refute_nil totals - assert_equal 7, totals[:primo] - assert_equal 3, totals[:timdex] + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch - assert res[:results].is_a?(Array) + assert_equal 2, result[:results].length + titles = result[:results].map { |r| r[:title] } + assert_includes titles, 'P1' + assert_includes titles, 'T1' end - test 'all tab cold cache deeper page calls timdex twice' do + test 'fetch appends only the new stable slice on load more' do + primo_calls = [] timdex_calls = [] + primo_fetcher = lambda do |offset:, per_page:, query: nil| + primo_calls << offset + results = [ + { title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }, + { title: 'P2', score: 0.8, api: 'primo', identifier: 'p2' }, + { title: 'P3', score: 0.7, api: 'primo', identifier: 'p3' }, + { title: 'P4', score: 0.6, api: 'primo', identifier: 'p4' } + ].slice(offset, per_page) || [] + { results: results, hits: 4, errors: nil, show_continuation: false } + end + timdex_fetcher = lambda do |offset:, per_page:, query: nil| + timdex_calls << offset + results = [ + { title: 'T1', score: 0.95, api: 'timdex', identifier: 't1' }, + { title: 'T2', score: 0.85, api: 'timdex', identifier: 't2' }, + { title: 'T3', score: 0.75, api: 'timdex', identifier: 't3' }, + { title: 'T4', score: 0.65, api: 'timdex', identifier: 't4' } + ].slice(offset, per_page) || [] + { results: results, hits: 4, errors: nil } + end - fake_timdex = fake_fetcher(results: %w[t1 t2 t3 t4 t5], hits: 150, calls: timdex_calls) - fake_primo = fake_fetcher(results: %w[p1 p2 p3], hits: 100) - service = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', - primo_fetcher: fake_primo, timdex_fetcher: fake_timdex) + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + + first = svc.fetch(display_count: 2, stable_count: 0, per_source: 1) + second = svc.fetch(display_count: 4, stable_count: 2, per_source: 1) - # Verify cache is cold - assert_nil Rails.cache.read(service.send(:totals_cache_key)) + assert_equal(first[:results].map { |result| result[:identifier] }, + second[:results].first(2).map { |result| result[:identifier] }) + assert_equal(second[:results].last(2).map { |result| result[:identifier] }, + second[:append_results].map { |result| result[:identifier] }) + assert_equal [0, 1], primo_calls + assert_equal [0, 1], timdex_calls + end - # Fetch page 7, verify TIMDEX was called twice - service.fetch(page: 7, per_page: 10) - assert_equal 2, timdex_calls.length + test 'fetch stops when duplicate source chunks do not grow the ordered result set' do + calls = [] + fetcher = lambda do |offset:, per_page:, query: nil| + calls << offset + { results: [{ title: 'Same', score: 1.0, api: 'primo', identifier: 'same' }], hits: 100, errors: nil } + end - # First call: summary (offset=0, per_page=1) - assert_equal({ offset: 0, per_page: 1 }, timdex_calls[0]) + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fetcher, timdex_fetcher: fake_fetcher) + result = svc.fetch(display_count: 5, stable_count: 0, per_source: 1) - # Second call: page data (offset/per_page computed by MergedSearchPaginator) - paginator = MergedSearchPaginator.new(primo_total: 100, timdex_total: 150, current_page: 7, per_page: 10) - expected_offset = paginator.api_offsets[1] - expected_count = paginator.merge_plan.count(:timdex) - assert_equal({ offset: expected_offset, per_page: expected_count }, timdex_calls[1]) + assert_equal 1, result[:results].length + assert_operator calls.length, :<=, 2 end - test 'fetch_all_tab_page_chunks handles zero-count branches' do - called = [] - primo_fetcher = lambda { |offset:, per_page:, query:| - called << [:primo, offset, per_page] - { results: ['P'], hits: 5, errors: nil, show_continuation: false } - } - timdex_fetcher = lambda { |offset:, per_page:, query:| - called << [:timdex, offset, per_page] + test 'fetch defaults to 50 results per source when env var is not set' do + per_page_seen = [] + fetcher = lambda do |offset:, per_page:, query: nil| + per_page_seen << per_page { results: [], hits: 0, errors: nil } - } + end - svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fetcher, timdex_fetcher: fetcher) + ClimateControl.modify(ALL_TAB_RESULTS_PER_SOURCE: nil) { svc.fetch } + + assert_equal [50, 50], per_page_seen + end + + test 'configured_scorer returns Zipper by default' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + ClimateControl.modify(ALL_TAB_SCORER: nil) do + assert_instance_of Reranker::ZipperMergeScorer, svc.send(:configured_scorer) + end + end + + test 'configured_scorer maps ALL_TAB_SCORER env var to correct scorer class' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + + { + 'zscore' => Reranker::ZscoreScorer, + 'zipper' => Reranker::ZipperMergeScorer, + 'simple' => Reranker::SimpleScorer, + 'random' => Reranker::RandomScorer + }.each do |scorer_name, scorer_class| + ClimateControl.modify(ALL_TAB_SCORER: scorer_name) do + assert_instance_of scorer_class, svc.send(:configured_scorer), + "Expected #{scorer_class} for ALL_TAB_SCORER=#{scorer_name}" + end + end + end + + test 'fetch combines errors from both sources' do + primo_fetcher = fake_fetcher(errors: [{ 'message' => 'Primo error' }]) + timdex_fetcher = fake_fetcher(errors: [{ 'message' => 'TIMDEX error' }]) + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch + + assert_equal 2, result[:errors].length + end - paginator = OpenStruct.new( - merge_plan: %i[primo primo], - api_offsets: [10, 0], - primo_total: 5, - timdex_total: 0 - ) - - primo_data, timdex_data = svc.send(:fetch_all_tab_page_chunks, paginator) - assert primo_data[:results].is_a?(Array) - assert timdex_data[:results].is_a?(Array) - assert_equal 0, timdex_data[:hits] + test 'fetch returns nil errors when both sources have no errors' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + assert_nil svc.fetch[:errors] end test 'combine_errors merges arrays or returns nil' do - svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) + svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) assert_nil svc.send(:combine_errors, nil, []) merged = svc.send(:combine_errors, [{ 'message' => 'a' }], [{ 'message' => 'b' }]) assert_equal 2, merged.length @@ -164,57 +217,24 @@ class MergedSearchServiceTest < ActiveSupport::TestCase # intentionally removed; the service now requires injected fetchers so # per-backend behavior should be tested in their respective unit tests. - test 'merge_results handles unbalanced API responses correctly' do - # Test case 1: Primo has fewer results than TIMDEX - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 5, current_page: 1, per_page: 8) - primo_results = %w[P1 P2 P3] - timdex_results = %w[T1 T2 T3 T4 T5] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 T4 T5] - assert_equal expected, merged - - # Test case 2: TIMDEX has fewer results than Primo - paginator = MergedSearchPaginator.new(primo_total: 5, timdex_total: 3, current_page: 1, per_page: 8) - primo_results = %w[P1 P2 P3 P4 P5] - timdex_results = %w[T1 T2 T3] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 P4 P5] - assert_equal expected, merged - - # Test case 3: Results exceed per_page limit (default 20) - paginator = MergedSearchPaginator.new(primo_total: 15, timdex_total: 15, current_page: 1, per_page: 20) - primo_results = (1..15).map { |i| "P#{i}" } - timdex_results = (1..15).map { |i| "T#{i}" } - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - assert_equal 20, merged.length - assert_equal 'P1', merged[0] - assert_equal 'T1', merged[1] - assert_equal 'P2', merged[2] - assert_equal 'T2', merged[3] - - # Test case 4: One array is empty - paginator = MergedSearchPaginator.new(primo_total: 0, timdex_total: 3, current_page: 1, per_page: 3) - primo_results = [] - timdex_results = %w[T1 T2 T3] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - assert_equal %w[T1 T2 T3], merged - - # Test case 5: more than 10 results from a single source can display when appropriate - paginator = MergedSearchPaginator.new(primo_total: 7, timdex_total: 11, current_page: 1, per_page: 18) - primo_results = (1..7).map { |i| "P#{i}" } - timdex_results = (1..11).map { |i| "T#{i}" } - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 P4 T4 P5 T5 P6 T6 P7 T7 T8 T9 T10 T11] - assert_equal expected, merged + test 'fetch handles unbalanced source responses and returns all available records' do + primo_all = (1..3).map { |i| { title: "P#{i}", score: 1.0 - (i * 0.01), api: 'primo', identifier: "p#{i}" } } + timdex_all = (1..5).map { |i| { title: "T#{i}", score: 1.0 - (i * 0.01), api: 'timdex', identifier: "t#{i}" } } + + primo_fetcher = lambda do |offset:, per_page:, query: nil| + { results: primo_all.slice(offset, per_page) || [], hits: primo_all.length, errors: nil, + show_continuation: false } + end + timdex_fetcher = lambda do |offset:, per_page:, query: nil| + { results: timdex_all.slice(offset, per_page) || [], hits: timdex_all.length, errors: nil } + end + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch(display_count: 8, stable_count: 0, per_source: 2) + + assert_equal 8, result[:results].length + ids = result[:results].map { |record| record[:identifier] }.sort + assert_equal %w[p1 p2 p3 t1 t2 t3 t4 t5], ids end end