diff --git a/README.md b/README.md index 0ffc090f..a77eac57 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,8 @@ 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 - `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. diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6bcec29c..50e9c8b5 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base def set_active_tab # GeoData doesn't use the tab system. return if Feature.enabled?(:geodata) - + @active_tab = if params[:tab].present? && valid_tab?(params[:tab]) # If params[:tab] is set and valid, use it and set session cookies[:last_tab] = params[:tab] @@ -21,9 +21,21 @@ def set_active_tab end end + def primo_tabs + %w[alma cdi primo] + end + + def timdex_tabs + %w[aspace timdex timdex_alma website] + end + + def all_tabs + ['all', *primo_tabs, *timdex_tabs] + end + private def valid_tab?(tab) - %w[primo timdex all].include?(tab) + all_tabs.include?(tab) end end diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 3157eab0..47e50fcc 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -23,15 +23,15 @@ def results render 'results_geo' return end - + # Route to appropriate search based on active tab case @active_tab - when 'primo' - load_primo_results - when 'timdex' - load_timdex_results when 'all' load_all_results + when *primo_tabs + load_primo_results + when *timdex_tabs + load_timdex_results end end @@ -108,7 +108,7 @@ def load_all_results # For now, just use primo pagination as a placeholder @pagination = primo_data[:pagination] || {} - + # Handle primo continuation for high page numbers @show_primo_continuation = primo_data[:show_continuation] || false end @@ -119,9 +119,7 @@ def fetch_primo_data offset = (current_page - 1) * per_page # Check if we're beyond Primo API limits before making the request. - if offset >= Analyzer::PRIMO_MAX_OFFSET - return { results: [], pagination: {}, errors: nil, show_continuation: true } - end + return { results: [], pagination: {}, errors: nil, show_continuation: true } if offset >= Analyzer::PRIMO_MAX_OFFSET primo_response = query_primo results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize @@ -133,7 +131,7 @@ def fetch_primo_data # exists in Primo UI, sending users there seems like the best we can do. show_continuation = false errors = nil - + if results.empty? docs = primo_response['docs'] if primo_response.is_a?(Hash) if docs.nil? || docs.empty? @@ -153,7 +151,7 @@ def fetch_timdex_data query = QueryBuilder.new(@enhanced_query).query response = query_timdex(query) errors = extract_errors(response) - + if errors.nil? pagination = Analyzer.new(@enhanced_query, response, :timdex).pagination raw_results = extract_results(response) @@ -169,6 +167,10 @@ def active_filters end def query_timdex(query) + query[:sourceFilter] = ['MIT Libraries Website', 'LibGuides'] if @active_tab == 'website' + query[:sourceFilter] = ['MIT ArchivesSpace'] if @active_tab == 'aspace' + query[:sourceFilter] = ['MIT Alma'] if @active_tab == 'timdex_alma' + # We generate unique cache keys to avoid naming collisions. cache_key = generate_cache_key(query) @@ -176,7 +178,7 @@ def query_timdex(query) Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do raw = if Feature.enabled?(:geodata) execute_geospatial_query(query) - elsif @active_tab == 'timdex' || @active_tab == 'all' + else TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query) end diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index b8f4ea34..e4d839a8 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -22,16 +22,22 @@ def link_to_result(result) end end - def link_to_tab(target) - if @active_tab == target.downcase - link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)), - aria: { current: "page" }, - class: "active tab-link", - data: { turbo_frame: "search-results", turbo_action: "advance" } + # Generates a link for a search results tab, marking it as active if it matches the current active tab. + # @param target [String] The name of the tab to link to + # @param label [String] The display label for the tab (optional) + # @return [String] HTML link element for the tab + def link_to_tab(target, label = nil) + clean_target = target.downcase.gsub(' ', '_').downcase + tab_label = label || target + if @active_tab == clean_target + link_to tab_label, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: clean_target)), + aria: { current: 'page' }, + class: 'active tab-link', + data: { turbo_frame: 'search-results', turbo_action: 'advance' } else - link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)), - class: "tab-link", - data: { turbo_frame: "search-results", turbo_action: "advance" } + link_to tab_label, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: clean_target)), + class: 'tab-link', + data: { turbo_frame: 'search-results', turbo_action: 'advance' } end end diff --git a/app/models/feature.rb b/app/models/feature.rb index b36fd77e..1a40e35d 100644 --- a/app/models/feature.rb +++ b/app/models/feature.rb @@ -33,7 +33,7 @@ # class Feature # List of all valid features in the application - VALID_FEATURES = %i[geodata boolean_picker simulate_search_latency].freeze + VALID_FEATURES = %i[geodata boolean_picker simulate_search_latency tab_timdex_all tab_timdex_alma].freeze # Check if a feature is enabled by name # diff --git a/app/models/normalize_primo_record.rb b/app/models/normalize_primo_record.rb index cac88518..b142526a 100644 --- a/app/models/normalize_primo_record.rb +++ b/app/models/normalize_primo_record.rb @@ -8,6 +8,7 @@ def initialize(record, query) def normalize { # Core fields + api: 'primo', title:, creators:, source:, diff --git a/app/models/normalize_timdex_record.rb b/app/models/normalize_timdex_record.rb index 9851e9e0..d09cfafc 100644 --- a/app/models/normalize_timdex_record.rb +++ b/app/models/normalize_timdex_record.rb @@ -8,6 +8,7 @@ def initialize(record, query) def normalize { # Core fields + api: 'timdex', title:, creators:, source:, diff --git a/app/views/search/_source_tabs.html.erb b/app/views/search/_source_tabs.html.erb index d0f73c17..c1d23ebc 100644 --- a/app/views/search/_source_tabs.html.erb +++ b/app/views/search/_source_tabs.html.erb @@ -2,8 +2,15 @@ \ No newline at end of file diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index eee01f2e..d259cd73 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -22,21 +22,14 @@

From all MIT Libraries sources

-
    - <% case @active_tab %> - <% when 'primo' %> - <%= render(partial: 'search/result_primo', collection: @results, as: :result) %> - <% when 'timdex' %> - <%= render(partial: 'search/result', collection: @results, as: :result) %> - <% when 'all' %> - <% @results.each do |result| %> - <% if result[:source] == 'Primo' %> +
      + <% @results.each do |result| %> + <% if result[:api] == 'primo' %> <%= render(partial: 'search/result_primo', locals: { result: result }) %> - <% else %> + <% elsif result[:api] == 'timdex' %> <%= render(partial: 'search/result', locals: { result: result }) %> <% end %> <% end %> - <% end %>
    <%= render partial: "results_callouts" %>
diff --git a/test/controllers/application_controller_unit_test.rb b/test/controllers/application_controller_unit_test.rb new file mode 100644 index 00000000..99b3eace --- /dev/null +++ b/test/controllers/application_controller_unit_test.rb @@ -0,0 +1,66 @@ +require 'test_helper' + +class ApplicationControllerUnitTest < ActionController::TestCase + setup do + @controller = ApplicationController.new + end + + test '#primo_tabs returns expected tabs' do + assert_equal %w[alma cdi primo], @controller.primo_tabs + end + + test '#timdex_tabs returns expected tabs' do + assert_equal %w[aspace timdex timdex_alma website], @controller.timdex_tabs + end + + test '#all_tabs includes both primo and timdex tabs as well as all tab' do + expected = %w[all alma cdi primo aspace timdex timdex_alma website] + assert_equal expected, @controller.all_tabs + end + + test '#valid_tab? returns true for valid tabs' do + assert @controller.send(:valid_tab?, 'all') + assert @controller.send(:valid_tab?, 'alma') + assert @controller.send(:valid_tab?, 'timdex') + end + + test '#valid_tab? returns false for invalid tabs' do + refute @controller.send(:valid_tab?, 'invalid') + refute @controller.send(:valid_tab?, '') + end + + test 'set_active_tab defaults @active_tab to all when no params or cookies are set' do + @controller.stubs(:params).returns({}) + @controller.stubs(:cookies).returns({}) + @controller.set_active_tab + assert_equal 'all', @controller.instance_variable_get(:@active_tab) + end + + test 'set_active_tab sets @active_tab to tab when tab params is valid' do + @controller.stubs(:params).returns({ tab: 'primo' }) + @controller.stubs(:cookies).returns({}) + @controller.set_active_tab + assert_equal 'primo', @controller.instance_variable_get(:@active_tab) + end + + test 'set_active_tab sets @active_tab to all when tab params is invalid and no cookie is set' do + @controller.stubs(:params).returns({ tab: 'supertab' }) + @controller.stubs(:cookies).returns({}) + @controller.set_active_tab + assert_equal 'all', @controller.instance_variable_get(:@active_tab) + end + + test 'set_active_tab sets @active_tab to cookie value when tab params is invalid and valid cookie is set' do + @controller.stubs(:params).returns({ tab: 'supertab' }) + @controller.stubs(:cookies).returns({ last_tab: 'timdex' }) + @controller.set_active_tab + assert_equal 'timdex', @controller.instance_variable_get(:@active_tab) + end + + test 'set_active_tab sets @active_tab to all value when tab params is invalid and cookie is invalid' do + @controller.stubs(:params).returns({ tab: 'supertab' }) + @controller.stubs(:cookies).returns({ last_tab: 'woohoo' }) + @controller.set_active_tab + assert_equal 'all', @controller.instance_variable_get(:@active_tab) + end +end diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index c652d1be..7a33dd13 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -4,6 +4,7 @@ class SearchControllerTest < ActionDispatch::IntegrationTest def mock_primo_search_success # Mock the Primo search components to avoid external API calls sample_doc = { + api: 'primo', title: 'Sample Primo Document Title', format: 'Article', year: '2025', @@ -26,6 +27,7 @@ def mock_primo_search_success def mock_timdex_search_success # Mock the TIMDEX GraphQL client to avoid external API calls sample_result = { + 'api' => 'timdex', 'title' => 'Sample TIMDEX Document Title', 'timdexRecordId' => 'sample-record-123', 'contentType' => [{ 'value' => 'Article' }], @@ -593,12 +595,24 @@ def source_filter_count(controller) assert_select 'a.tab-link.active[href*="tab=primo"]', count: 1 end - test 'results respects timdex tab parameter' do - mock_timdex_search_success + test 'results respects timdex all tab parameter' do + ClimateControl.modify FEATURE_TAB_TIMDEX_ALL: 'true' do + mock_timdex_search_success - get '/results?q=test&tab=timdex' - assert_response :success - assert_select 'a.tab-link.active[href*="tab=timdex"]', count: 1 + get '/results?q=test&tab=timdex' + assert_response :success + assert_select 'a.tab-link.active[href*="tab=timdex"]', count: 1 + end + end + + test 'results respects timdex alma tab parameter' do + ClimateControl.modify FEATURE_TAB_TIMDEX_ALMA: 'true' do + mock_timdex_search_success + + get '/results?q=test&tab=timdex_alma' + assert_response :success + assert_select 'a.tab-link.active[href*="tab=timdex_alma"]', count: 1 + end end test 'results shows tab navigation when GeoData is disabled' do @@ -608,7 +622,9 @@ def source_filter_count(controller) assert_response :success assert_select '.tab-navigation', count: 1 assert_select 'a[href*="tab=primo"]', count: 1 - assert_select 'a[href*="tab=timdex"]', 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 end test 'results handles primo search errors gracefully' do @@ -726,7 +742,7 @@ def source_filter_count(controller) get '/results?q=test&tab=all' assert_response :success - + # Verify that we get results from both sources assert_select '.record-title', text: /Sample Primo Document Title/ assert_select '.record-title', text: /Sample TIMDEX Document Title/ @@ -768,7 +784,7 @@ def source_filter_count(controller) 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: /Continue your search in Search Our Collections/ diff --git a/test/helpers/search_helper_test.rb b/test/helpers/search_helper_test.rb index 88402627..a4c9297e 100644 --- a/test/helpers/search_helper_test.rb +++ b/test/helpers/search_helper_test.rb @@ -203,7 +203,7 @@ class SearchHelperTest < ActionView::TestCase @active_tab = 'bar' actual = link_to_tab('Foo') - assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link| + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| assert_select '[class*=?]', 'active', count: 0 assert_select '[class*=?]', 'tab-link' assert_select '[aria-current=?]', 'page', count: 0 @@ -214,7 +214,7 @@ class SearchHelperTest < ActionView::TestCase @active_tab = 'foo' actual = link_to_tab('Foo') - assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link| + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| assert_select link, '[class*=?]', 'active' assert_select link, '[aria-current=?]', 'page', count: 1 end @@ -231,4 +231,36 @@ class SearchHelperTest < ActionView::TestCase expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/search?query=any%2Ccontains%2Cdata+%26+analytics&vid=01MIT_INST%3AMIT' assert_equal expected_url, primo_search_url(query) end + + test 'tab label defaults to target when label is nil' do + @active_tab = 'sample_tab' + actual = link_to_tab('Sample Tab', nil) + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| + assert_equal link.text, 'Sample Tab' + end + end + + test 'tab label uses label when label is provided' do + @active_tab = 'sample_tab' + actual = link_to_tab('Sample Tab', 'Custom Label') + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| + assert_equal link.text, 'Custom Label' + end + end + + test 'tab param uses clean_target when label is nil' do + @active_tab = 'sample_tab' + actual = link_to_tab('Sample Tab', nil) + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| + assert_equal link.first[:href], '/results?tab=sample_tab' + end + end + + test 'tab param uses clean_target when label is provided' do + @active_tab = 'sample_tab' + actual = link_to_tab('Sample Tab', 'Custom Label') + assert_select Nokogiri::HTML::Document.parse(actual), 'a' do |link| + assert_equal link.first[:href], '/results?tab=sample_tab' + end + end end diff --git a/test/models/normalize_primo_record_test.rb b/test/models/normalize_primo_record_test.rb index b8e1740d..bc6ebd55 100644 --- a/test/models/normalize_primo_record_test.rb +++ b/test/models/normalize_primo_record_test.rb @@ -423,4 +423,9 @@ def cdi_record assert_not_nil dedup_url assert_match %r{/discovery/search\?}, dedup_url end + + test 'includes primo as source api' do + normalized = NormalizePrimoRecord.new(full_record, 'test query').normalize + assert_equal 'primo', normalized[:api] + end end diff --git a/test/models/normalize_primo_results_test.rb b/test/models/normalize_primo_results_test.rb index f1d3ff16..e71bce62 100644 --- a/test/models/normalize_primo_results_test.rb +++ b/test/models/normalize_primo_results_test.rb @@ -67,4 +67,13 @@ def empty_results normalizer = NormalizePrimoResults.new(nil, 'test query') assert_equal 0, normalizer.total_results end + + test 'all primo results includes primo as source api' do + normalizer = NormalizePrimoResults.new(sample_results, 'test query') + normalized = normalizer.normalize + + normalized.each do |result| + assert_equal 'primo', result[:api] + end + end end diff --git a/test/models/normalize_timdex_record_test.rb b/test/models/normalize_timdex_record_test.rb index d3540603..fc60aafe 100644 --- a/test/models/normalize_timdex_record_test.rb +++ b/test/models/normalize_timdex_record_test.rb @@ -218,4 +218,9 @@ def minimal_record assert_not_includes normalized.keys, 'other_availability' assert_not_includes normalized.keys, 'container' end + + test 'includes timdex as source api' do + normalized = NormalizeTimdexRecord.new(full_record, 'test').normalize + assert_equal 'timdex', normalized[:api] + end end diff --git a/test/models/normalize_timdex_results_test.rb b/test/models/normalize_timdex_results_test.rb index c7e71267..8b237690 100644 --- a/test/models/normalize_timdex_results_test.rb +++ b/test/models/normalize_timdex_results_test.rb @@ -85,4 +85,13 @@ def empty_timdex_response # Test that query is stored (this would be used by individual record normalization) assert_equal query, normalizer.instance_variable_get(:@query) end + + test 'all timdex results includes timdex as source api' do + normalizer = NormalizeTimdexResults.new(sample_timdex_response, 'test query') + normalized = normalizer.normalize + + normalized.each do |result| + assert_equal 'timdex', result[:api] + end + end end