Merge pull request #286 from MITLibraries/use-202-primo-tabs

Adds Primo API-based tabs

Author: JPrevost

README.md

@@ -79,9 +79,9 @@ See `Optional Environment Variables` for more information.
 - `MIT_PRIMO_URL`: The base URL for MIT Libraries' Primo instance (used to generate record links).
 - `PRIMO_API_KEY`: The Primo Search API key.
 - `PRIMO_API_URL`: The Primo Search API base URL.
-- `PRIMO_SCOPE`: The Primo Search API `scope` param (set to `cdi` for CDI-scoped results).
-- `PRIMO_TAB`: The Primo Search API `tab` param (typically `all`).
-- `PRIMO_VID`: The Primo Search API `vid` (or 'view ID`) param.
+- `PRIMO_SCOPE`: The Primo Search API `scope` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
+- `PRIMO_TAB`: The Primo Search API `tab` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
+- `PRIMO_VID`: The Primo Search API `vid` (or 'view ID`) param. Used in both our API calls and Primo UI. Ask Enterprise Systems for value.
 - `SECRET_KEY_BASE`: You can generate this via `bin/rails secret`. Please do not re-use the production value locally.
 - `SYNDETICS_PRIMO_URL`: The Syndetics API URL for Primo. This is used to construct thumbnail URLs.
 - `TIMDEX_GRAPHQL`: Set this to the URL of the GraphQL endpoint. There is no default value in the application.
@@ -98,8 +98,9 @@ See `Optional Environment Variables` for more information.
 mode. Note that this is currently intended _only_ for the geodata app and
 may have unexpected consequences if applied to other TIMDEX UI apps.
 - `FEATURE_SIMULATE_SEARCH_LATENCY`: DO NOT SET IN PRODUCTION. Set to ensure a minimum of a one second delay in returning search results. Useful to see spinners/loaders. Only introduces delay for results that take less than one second to complete.
-- `FEATURE_TAB_TIMDEX_ALL`: Display a tab for displaying the combined TIMDEX data
-- `FEATURE_TAB_TIMDEX_ALMA`: Display a tab for displaying Alma data from TIMDEX
+- `FEATURE_TAB_PRIMO_ALL`: Display a tab for displaying the combined Primo data (CDI + Alma)
+- `FEATURE_TAB_TIMDEX_ALL`: Display a tab for displaying the combined TIMDEX data. `TIMDEX_INDEX` affects which data appears in this tab.
+- `FEATURE_TAB_TIMDEX_ALMA`: Display a tab for displaying Alma data from TIMDEX. `TIMDEX_INDEX` must include `Alma` data or no results will return.
 - `FILTER_ACCESS_TO_FILES`: The name to use instead of "Access to files" for that filter / aggregation.
 - `FILTER_CONTENT_TYPE`: The name to use instead of "Content type" for that filter / aggregation.
 - `FILTER_CONTRIBUTOR`: The name to use instead of "Contributor" for that filter / aggregation.

app/controllers/search_controller.rb

@@ -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 , :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
@@ -226,7 +226,7 @@ def query_primo(per_page, offset)
     cache_key = generate_cache_key(@enhanced_query)
 
     Rails.cache.fetch("#{cache_key}/primo", expires_in: 12.hours) do
-      primo_search = PrimoSearch.new
+      primo_search = PrimoSearch.new(@enhanced_query[:tab])
       primo_search.search(@enhanced_query[:q], per_page, offset)
     end
   end

app/models/feature.rb

@@ -33,7 +33,8 @@
 #
 class Feature
   # List of all valid features in the application
-  VALID_FEATURES = %i[geodata boolean_picker simulate_search_latency tab_timdex_all tab_timdex_alma].freeze
+  VALID_FEATURES = %i[geodata boolean_picker simulate_search_latency tab_primo_all tab_timdex_all
+                      tab_timdex_alma].freeze
 
   # Check if a feature is enabled by name
   #

app/models/primo_search.rb

@@ -1,12 +1,23 @@
 # Searches Primo Search API and formats results
 #
 class PrimoSearch
-  def initialize
+  # Initializes PrimoSearch
+  # @param tab [String] Current `active_tab` value from SearchController. Used to set Primo tab/scope.
+  #   Defaults to 'all'.
+  # @return [PrimoSearch] An instance of PrimoSearch
+  def initialize(tab = 'all')
+    @tab = tab
+
     validate_env
     @primo_http = HTTP.persistent(primo_api_url)
     @results = {}
   end
 
+  # Performs a search against the Primo API
+  # @param term [String] The search term
+  # @param per_page [Integer] Number of results per page
+  # @param offset [Integer] The result offset for pagination
+  # @return [Hash] Parsed JSON response from Primo API
   def search(term, per_page, offset = 0)
     url = search_url(term, per_page, offset)
     result = @primo_http.timeout(http_timeout)
@@ -23,13 +34,15 @@ def search(term, per_page, offset = 0)
 
   private
 
+  # Validates required environment variables are set
+  #
+  # PRIMO_SCOPE and PRIMO_TAB look related by name, but are only used in the links to Primo UI and cannot be the same
+  # values we use for API calls so are not checked here.
   def validate_env
     missing_vars = []
 
     missing_vars << 'PRIMO_API_URL' if primo_api_url.nil?
     missing_vars << 'PRIMO_API_KEY' if primo_api_key.nil?
-    missing_vars << 'PRIMO_SCOPE' if primo_scope.nil?
-    missing_vars << 'PRIMO_TAB' if primo_tab.nil?
     missing_vars << 'PRIMO_VID' if primo_vid.nil?
 
     return if missing_vars.empty?
@@ -46,14 +59,40 @@ def primo_api_key
     ENV.fetch('PRIMO_API_KEY', nil)
   end
 
+  # In Primo API, Search scopes determines which records are being searched
+  # Primo VE configuration calls this `Search Profiles` and uses `Scope` differently.
+  # For Primo VE API we want to be doing &scope={Primo VE Search Profile}
+  #
+  # Available scopes for USE
+  #   all_use: includes CDI configured like Primo UI + Alma (does not include ASpace)
+  #   all: same as all_use but includes ASpace
+  #   catalog_use: just Alma
+  #   cdi_use: just CDI configured like Primo UI
+  #
+  # The scope we use will be driven by the tab provided during initialization
   def primo_scope
-    ENV.fetch('PRIMO_SCOPE', nil)
+    case @tab
+    when 'cdi'
+      'cdi_use'
+    when 'alma'
+      'catalog_use'
+    else
+      'all_use'
+    end
   end
 
+  # In Primo, Tabs act as "search scope slots". They contain one or more Search Profile (which Primo API calls `scopes`).
+  # Primo VE configuration refers to these as `Search Profile Slots`
+  # Configured tabs in our Primo
+  #   all: scopes(all, all_filtered, catalog, cdi, CourseReserves)
+  #   bento: scopes(cdi, catalog, bento_catalog, all_use)
+  #   USE: scopes(all_use, all, catalog_use, cdi_use)
+  #   This application should always use the 'use' tab for Primo searches.
   def primo_tab
-    ENV.fetch('PRIMO_TAB', nil)
+    'use'
   end
 
+  # In Primo API, a view (vid) contains Search Profile Slots (tabs) which in turn contain Search Profiles (scopes).
   def primo_vid
     ENV.fetch('PRIMO_VID', nil)
   end

app/views/search/_source_tabs.html.erb

@@ -2,7 +2,14 @@
   <nav id="tabs" class="tab-navigation" aria-label="Result type navigation">
     <ul>
       <li><%= link_to_tab("All") %></li>
-      <li><%= link_to_tab("Primo", "Articles and Catalog") %></li>
+
+      <% if Feature.enabled?(:tab_primo_all) %>
+        <li><%= link_to_tab("Primo", "Articles and Catalog") %></li>
+      <% end %>
+
+      <li><%= link_to_tab("cdi", "Articles") %></li>
+      <li><%= link_to_tab("alma", "Catalog") %></li>
+
       <% if Feature.enabled?(:tab_timdex_all) %>
         <li><%= link_to_tab("TIMDEX") %></li>
       <% end %>

test/controllers/search_controller_test.rb

@@ -655,11 +655,13 @@ def source_filter_count(controller)
   end
 
   test 'results respects primo tab parameter' do
-    mock_primo_search_success
+    ClimateControl.modify FEATURE_TAB_PRIMO_ALL: 'true' do
+      mock_primo_search_success
 
-    get '/results?q=test&tab=primo'
-    assert_response :success
-    assert_select 'a.tab-link.active[href*="tab=primo"]', count: 1
+      get '/results?q=test&tab=primo'
+      assert_response :success
+      assert_select 'a.tab-link.active[href*="tab=primo"]', count: 1
+    end
   end
 
   test 'results respects timdex all tab parameter' do
@@ -688,7 +690,10 @@ def source_filter_count(controller)
     get '/results?q=test&tab=primo'
     assert_response :success
     assert_select '.tab-navigation', count: 1
-    assert_select 'a[href*="tab=primo"]', count: 1
+    assert_select 'a[href*="tab=all"]', count: 1
+    assert_select 'a[href*="tab=cdi"]', count: 1
+    assert_select 'a[href*="tab=alma"]', count: 1
+    # assert_select 'a[href*="tab=primo"]', count: 1
     # assert_select 'a[href*="tab=timdex"]', count: 1
     assert_select 'a[href*="tab=aspace"]', count: 1
     assert_select 'a[href*="tab=website"]', count: 1

test/models/primo_search_test.rb

@@ -28,12 +28,11 @@ class PrimoSearchTest < ActiveSupport::TestCase
   end
 
   test 'raises error when multiple environment variables are missing' do
-    ClimateControl.modify(PRIMO_API_URL: nil, PRIMO_SCOPE: nil, PRIMO_VID: nil) do
+    ClimateControl.modify(PRIMO_API_URL: nil, PRIMO_VID: nil) do
       error = assert_raises(ArgumentError) do
         PrimoSearch.new
       end
       assert_match(/PRIMO_API_URL/, error.message)
-      assert_match(/PRIMO_SCOPE/, error.message)
       assert_match(/PRIMO_VID/, error.message)
     end
   end