From b2d97627be924b57c53d39f777a40e432751b9da Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Fri, 3 Oct 2025 09:44:14 -0400 Subject: [PATCH] Initial tabbed Primo/TIMDEX interface Why these changes are being introduced: USE UI needs affordances to switch between results from different sources. Primo results should be the default view. Relevant ticket(s): * [USE-31](https://mitlibraries.atlassian.net/browse/USE-31) * [TIMX-549](https://mitlibraries.atlassian.net/browse/TIMX-549) How this addresses that need: * Integrates Primo search into the controller and view layers * Adds Turbo frame 'tabs' to switch between Primo and TIMDEX results. Side effects of this change: * Maintaining separate views for each result type is probably not ideal, but I'm accepting it as a risk until we normalize TIMDEX records similarly to how we normalize Primo. * Several tests have been skipped for features that are no longer relevant to USE UI, but are core to GDT. We will need to revise our overall test strategy such that these features are tested as part of GDT. * FRBRized full record links don't always work. It the method we use breaks FRBR links for CDI records. Predictably, Ex Libris documentation on FRBR does not address this issue. We might decide to forgo FRBR links in general, or perhaps limit them by content type (assuming book results always come from Alma). * Pagination is not yet implemented. --- app/controllers/search_controller.rb | 127 +++-- app/views/search/_form.html.erb | 251 ++++----- app/views/search/_result_primo.html.erb | 77 +++ app/views/search/_search_summary_use.html.erb | 7 + app/views/search/results.html.erb | 89 +-- test/controllers/search_controller_test.rb | 525 +++++++++++------- test/integration/error_resilience_test.rb | 2 +- 7 files changed, 675 insertions(+), 403 deletions(-) create mode 100644 app/views/search/_result_primo.html.erb create mode 100644 app/views/search/_search_summary_use.html.erb diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index f1efd795..02772968 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -15,59 +15,102 @@ def results # inject session preference for boolean type if it is present params[:booleanType] = cookies[:boolean_type] || 'AND' - # hand off to Enhancer chain + # Determine which tab to load - default to primo unless gdt is enabled + @active_tab = if Flipflop.enabled?(:gdt) + 'gdt' # Keep existing GDT behavior unchanged + else + params[:tab] || 'primo' # Default to primo for new tabbed interface + end @enhanced_query = Enhancer.new(params).enhanced_query - # hand off enhanced query to builder - query = QueryBuilder.new(@enhanced_query).query + # Route to appropriate search based on active tab + if Flipflop.enabled?(:gdt) + # Keep existing GDT behavior unchanged + load_gdt_results + else + case @active_tab + when 'primo' + load_primo_results + when 'timdex' + load_timdex_results + end + end + end - # Create cache key for this query - # Sorting query hash to ensure consistent key generation regardless of the parameter order - sorted_query = query.sort_by { |k, v| k.to_sym }.to_h - cache_key = Digest::MD5.hexdigest(sorted_query.to_s) + private - # builder hands off to wrapper which returns raw results here - # We are using two difference caches to allow for Geo and USE to be cached separately. This ensures we don't have - # cache key collission for these two different query types. In practice, the likelihood of this happening is low, - # as the query parameters are different for each type and they won't often be run with the same cache backend other - # than locally, but this is a safeguard. - # The response type is a GraphQL::Client::Response, which is not directly serializable, so we convert it to a hash. - response = if Flipflop.enabled?(:gdt) - Rails.cache.fetch("#{cache_key}/geo", expires_in: 12.hours) do - raw = execute_geospatial_query(query) - { - data: raw.data.to_h, - errors: raw.errors.details.to_h - } - end - else - Rails.cache.fetch("#{cache_key}/use", expires_in: 12.hours) do - raw = TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query) - { - data: raw.data.to_h, - errors: raw.errors.details.to_h - } - end - end + def load_gdt_results + query = QueryBuilder.new(@enhanced_query).query + + response = cache_timdex_query(query) # Handle errors @errors = extract_errors(response) - - # Analayze results - # The @pagination instance variable includes info about next/previous pages (where they exist) to assist the UI. @pagination = Analyzer.new(@enhanced_query, response).pagination if @errors.nil? - - # Display results @results = extract_results(response) @filters = extract_filters(response) end - private + def load_primo_results + begin + primo_search = PrimoSearch.new + per_page = params[:per_page] || 20 + primo_response = primo_search.search(params[:q], per_page) + + @results = NormalizePrimoResults.new(primo_response, params[:q]).normalize + + # Basic pagination for now. + if @results.present? + @pagination = { + hits: @results.count, + start: 1, + end: @results.count + } + end + + rescue StandardError => e + @errors = handle_primo_errors(e) + end + end + + def load_timdex_results + query = QueryBuilder.new(@enhanced_query).query + response = cache_timdex_query(query) + + @errors = extract_errors(response) + @pagination = Analyzer.new(@enhanced_query, response).pagination if @errors.nil? + @results = extract_results(response) + end def active_filters ENV.fetch('ACTIVE_FILTERS', '').split(',').map(&:strip) end + def cache_timdex_query(query) + # Create cache key for this query + # Sorting query hash to ensure consistent key generation regardless of the parameter order + sorted_query = query.sort_by { |k, v| k.to_sym }.to_h + cache_key = Digest::MD5.hexdigest(sorted_query.to_s) + + # builder hands off to wrapper which returns raw results here + # We are using two difference caches to allow for Geo and USE to be cached separately. This ensures we don't have + # cache key collision for these two different query types. In practice, the likelihood of this happening is low, + # as the query parameters are different for each type and they won't often be run with the same cache backend other + # than locally, but this is a safeguard. + # The response type is a GraphQL::Client::Response, which is not directly serializable, so we convert it to a hash. + Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do + raw = if @active_tab == 'gdt' + execute_geospatial_query(query) + elsif @active_tab == 'timdex' + TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query) + end + { + data: raw.data.to_h, + errors: raw.errors.details.to_h + } + end + end + def execute_geospatial_query(query) if query['geobox'] == 'true' && query[:geodistance] == 'true' TimdexBase::Client.query(TimdexSearch::AllQuery, variables: query) @@ -214,4 +257,16 @@ def validate_geobox_values! flash[:error] = 'Maximum latitude cannot exceed minimum latitude.' redirect_to root_url end + + def handle_primo_errors(error) + Rails.logger.error("Primo search error: #{error.message}") + + if error.is_a?(ArgumentError) + [{ 'message' => 'Primo search is not properly configured.' }] + elsif error.is_a?(HTTP::TimeoutError) + [{ 'message' => 'The Primo service is currently slow to respond. Please try again.' }] + else + [{ 'message' => error.message }] + end + end end diff --git a/app/views/search/_form.html.erb b/app/views/search/_form.html.erb index b0287460..81f02734 100644 --- a/app/views/search/_form.html.erb +++ b/app/views/search/_form.html.erb @@ -31,118 +31,117 @@ keyword_placeholder = search_required ? "Enter your search" : "Keyword anywhere" <%= 'aria-describedby=site-desc' if Flipflop.enabled?(:gdt) %>> <% if Flipflop.enabled?(:gdt) %> -
> - - <%= geobox_label %> - - -
- Search within a geospatial bounding box -

* All fields in this section are required

-
-
- - " - id="geobox-minLongitude" name="geoboxMinLongitude" value="<%= params[:geoboxMinLongitude] %>" - <%= 'required' if geobox_required %> - aria-describedby="minLong-desc"> - - A decimal between -180.0 and 180.0 (Ex: -73.507239) +
> + + <%= geobox_label %> + + +
+ Search within a geospatial bounding box +

* All fields in this section are required

+
+
+ + " + id="geobox-minLongitude" name="geoboxMinLongitude" value="<%= params[:geoboxMinLongitude] %>" + <%= 'required' if geobox_required %> + aria-describedby="minLong-desc"> + + A decimal between -180.0 and 180.0 (Ex: -73.507239) + Western Hemisphere is negative + +
+
+ + " + id="geobox-minLatitude" name="geoboxMinLatitude" value="<%= params[:geoboxMinLatitude] %>" + <%= 'required' if geobox_required %> + aria-describedby="minLat-desc"> + + A decimal between -90.0 and 90.0 (Ex: 41.239083) + Southern Hemisphere is negative + +
+
+ + " + id="geobox-maxLongitude" name="geoboxMaxLongitude" value="<%= params[:geoboxMaxLongitude] %>" + <%= 'required' if geobox_required %> + aria-describedby="maxLong-desc"> + + A decimal between -180.0 and 180.0 (Ex: -69.928713) Western Hemisphere is negative -
-
- - " - id="geobox-minLatitude" name="geoboxMinLatitude" value="<%= params[:geoboxMinLatitude] %>" - <%= 'required' if geobox_required %> - aria-describedby="minLat-desc"> - - A decimal between -90.0 and 90.0 (Ex: 41.239083) - Southern Hemisphere is negative - -
-
- +
+
+ + " + id="geobox-maxLatitude" name="geoboxMaxLatitude" value="<%= params[:geoboxMaxLatitude] %>" + <%= 'required' if geobox_required %> + aria-describedby="maxLat-desc"> + + A decimal between -90.0 and 90.0 (Ex: 42.886759) + Southern Hemisphere is negative + +
+
+
+
+
> + + <%= geodistance_label %> + + +
+ Search within a distance of a geographic point +

* All fields in this section are required

+
+
+ + " + id="geodistance-latitude" name="geodistanceLatitude" + value="<%= params[:geodistanceLatitude] %>" aria-describedby="lat-desc" + <%= 'required' if geodistance_required %> + aria-describedby="lat-desc"> + + A decimal between -90.0 and 90.0 (Ex: 42.279594) + Southern Hemisphere is negative + +
+
+ " - id="geobox-maxLongitude" name="geoboxMaxLongitude" value="<%= params[:geoboxMaxLongitude] %>" - <%= 'required' if geobox_required %> - aria-describedby="maxLong-desc"> - - A decimal between -180.0 and 180.0 (Ex: -69.928713) - Western Hemisphere is negative - -
-
- - " - id="geobox-maxLatitude" name="geoboxMaxLatitude" value="<%= params[:geoboxMaxLatitude] %>" - <%= 'required' if geobox_required %> - aria-describedby="maxLat-desc"> - - A decimal between -90.0 and 90.0 (Ex: 42.886759) - Southern Hemisphere is negative - -
+ class="field field-text <%= "required" if geodistance_required %>" + id="geodistance-longitude" name="geodistanceLongitude" + value="<%= params[:geodistanceLongitude] %>" aria-describedby="long-desc" + <%= 'required' if geodistance_required %> + aria-describedby="long-desc"> + + A decimal between -180.0 and 180.0 (Ex: -83.732124) + Western Hemisphere is negative +
-
-
-
> - - <%= geodistance_label %> - - -
- Search within a distance of a geographic point -

* All fields in this section are required

-
-
- - " - id="geodistance-latitude" name="geodistanceLatitude" - value="<%= params[:geodistanceLatitude] %>" aria-describedby="lat-desc" - <%= 'required' if geodistance_required %> - aria-describedby="lat-desc"> - - A decimal between -90.0 and 90.0 (Ex: 42.279594) - Southern Hemisphere is negative - -
-
- - " - id="geodistance-longitude" name="geodistanceLongitude" - value="<%= params[:geodistanceLongitude] %>" aria-describedby="long-desc" - <%= 'required' if geodistance_required %> - aria-describedby="long-desc"> - - A decimal between -180.0 and 180.0 (Ex: -83.732124) - Western Hemisphere is negative - -
-
- - " - id="geodistance-distance" name="geodistanceDistance" - value="<%= params[:geodistanceDistance] %>" aria-describedby="distance-desc" - <%= 'required' if geodistance_required %> - aria-describedby="distance-desc"> - - Distance is in meters by default; add other units if preferred (Ex: '100km' or '50mi') - -
+
+ + " + id="geodistance-distance" name="geodistanceDistance" + value="<%= params[:geodistanceDistance] %>" aria-describedby="distance-desc" + <%= 'required' if geodistance_required %> + aria-describedby="distance-desc"> + + Distance is in meters by default; add other units if preferred (Ex: '100km' or '50mi') +
-
-
- <% end %> +
+
+
> @@ -158,33 +157,11 @@ keyword_placeholder = search_required ? "Enter your search" : "Keyword anywhere"
- +
- <% unless Flipflop.enabled?(:gdt) %> -
- - -
- -
- - -
- -
- - -
- <% end %> -
- - <% unless Flipflop.enabled?(:gdt) %> -
- <%# https://www.w3.org/WAI/tutorials/forms/grouping/ %> -
- Limit search to checked sources. - - <% timdex_sources.each do |source| %> - <%= source_checkbox(source, params) %> - <% end %> -
-
- <% end %>
+ <% end %>
diff --git a/app/views/search/_result_primo.html.erb b/app/views/search/_result_primo.html.erb new file mode 100644 index 00000000..318eb664 --- /dev/null +++ b/app/views/search/_result_primo.html.erb @@ -0,0 +1,77 @@ +
  • +
    +

    + Title: + <% if result_primo['links']&.find { |link| link['kind'] == 'full record' } %> + <%= link_to(result_primo['title'], result_primo['links'].find { |link| link['kind'] == 'full record' }['url'], target: '_blank', rel: 'noopener') %> + <% else %> + <%= result_primo['title'] %> + <% end %> +

    + +

    + <%= result_primo['format'] %> + <%= result_primo['year'] %> +

    + + <% if result_primo['creators'].present? %> + Contributors: +
      + <% result_primo['creators'].each do |creator| %> +
    • + <% if creator[:link] %> + <%= link_to creator[:value], creator[:link] %> + <% else %> + <%= creator[:value] %> + <% end %> +
    • + <% end %> +
    + <% end %> + + <% if result_primo['container'].present? %> +

    + Published in: + <%= result_primo['container'] %> +

    + <% end %> + + <% if result_primo['citation'].present? %> +

    + Citation: + <%= result_primo['citation'] %> +

    + <% end %> + + <% if result_primo['summary'].present? %> +

    + Summary: + <%= truncate(result_primo['summary'], length: 300) %> +

    + <% end %> + + <% if result_primo['subjects'].present? %> +

    + Subjects: + <%= result_primo['subjects'].join('; ') %> +

    + <% end %> + + <% if result_primo['links'].present? %> + + <% end %> + + <% if result_primo['availability'].present? %> +

    + Availability: + <%= result_primo['availability'] %> +

    + <% end %> +
    +
  • \ No newline at end of file diff --git a/app/views/search/_search_summary_use.html.erb b/app/views/search/_search_summary_use.html.erb new file mode 100644 index 00000000..d59db3bd --- /dev/null +++ b/app/views/search/_search_summary_use.html.erb @@ -0,0 +1,7 @@ +<% return unless params[:q].present? %> + + \ No newline at end of file diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index aa89ac79..98f9cb51 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -9,7 +9,11 @@ <%= render partial: "shared/site_title" %> <%= render partial: "form" %> - <%= render partial: "search_summary" %> + <% if Flipflop.enabled?(:gdt) %> + <%= render partial: "search_summary" %> + <% else %> + <%= render partial: "search_summary_use" %> + <% end %> <% if ENV.fetch('FACT_PANELS_ENABLED', false).present? %>
    @@ -22,42 +26,59 @@ <%= render(partial: 'shared/error', collection: @errors) %> -
    - <% if @filters.present? %> - - <% end %> - -
    - <% if @results.present? %> -

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

    -
      - <% if Flipflop.enabled?(:gdt) %> - <%= render(partial: 'search/result_geo', collection: @results) %> - <% else %> - <%= render(partial: 'search/result', collection: @results) %> - <% end %> -
    - <% else %> -
    -

    No results found for your search

    -
    - <% end %> -
    + + <% elsif @errors.blank? %> +
    +

    No results found for your search

    +
    + <% end %> + +
    + <% end %> <%= render partial: 'shared/ask', locals: { display: 'aside' } if @results.blank? %> <% if @results.present? %> diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index c33385ed..6f3028bb 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -6,56 +6,128 @@ def setup @test_strategy.switch!(:gdt, false) end + def mock_primo_search_success + # Mock the Primo search components to avoid external API calls + sample_doc = { + '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], 'total' => 1 }) + PrimoSearch.expects(:new).returns(mock_primo) + + mock_normalizer = mock('normalizer') + mock_normalizer.expects(:normalize).returns([sample_doc]) + NormalizePrimoResults.expects(:new).returns(mock_normalizer) + end + + def mock_timdex_search_success + # Mock the TIMDEX GraphQL client to avoid external API calls + sample_result = { + 'title' => 'Sample TIMDEX Document Title', + 'timdexRecordId' => 'sample-record-123', + 'contentType' => [{ 'value' => 'Article' }], + 'dates' => [{ 'kind' => 'Publication date', 'value' => '2023' }], + 'contributors' => [{ 'value' => 'Foo Barston', 'kind' => 'Creator' }], + 'highlight' => [ + { + 'matchedField' => 'summary', + 'matchedPhrases' => ['sample document'] + } + ], + 'sourceLink' => 'https://example.com/record' + } + + mock_response = mock('timdex_response') + mock_errors = mock('timdex_errors') + mock_errors.stubs(:details).returns({}) + mock_errors.stubs(:to_h).returns({}) + mock_response.stubs(:errors).returns(mock_errors) + + mock_data = mock('timdex_data') + mock_search = mock('timdex_search') + mock_search.stubs(:to_h).returns({ + 'hits' => 1, + 'aggregations' => {}, + 'records' => [sample_result] + }) + mock_data.stubs(:search).returns(mock_search) + mock_data.stubs(:to_h).returns({ + 'search' => { + 'hits' => 1, + 'aggregations' => {}, + 'records' => [sample_result] + } + }) + mock_response.stubs(:data).returns(mock_data) + + TimdexBase::Client.expects(:query).returns(mock_response) + end + test 'index shows basic search form by default' do get '/' assert_response :success assert_select 'form#basic-search', { count: 1 } - - details_div = assert_select('details#advanced-search-panel') - assert_nil details_div.attribute('open') + assert_select 'details#advanced-search-panel', count: 0 end test 'index shows advanced search form with URL parameter' do - get '/?advanced=true' - - assert_response :success - - details_div = assert_select('details#advanced-search-panel') - assert details_div.attribute('open') - end - - test 'advanced search form appears on results page with URL parameter' do - VCR.use_cassette('advanced', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?advanced=true' - + if Flipflop.enabled?(:gdt) + get '/?advanced=true' assert_response :success - details_div = assert_select('details#advanced-search-panel') assert details_div.attribute('open') + else + skip('Advanced search functionality not implemented in USE UI') + end + end + + test 'advanced search form appears on results page with URL parameter' do + if Flipflop.enabled?(:gdt) + VCR.use_cassette('advanced', + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do + get '/results?advanced=true' + assert_response :success + details_div = assert_select('details#advanced-search-panel') + assert details_div.attribute('open') + end + else + skip('Advanced search functionality not implemented in USE UI') end end test 'search form includes a number of fields' do get '/' - # Please note that this test confirms fields in the DOM - but not whether - # they are visible. Fields in a hidden details panel are still in the DOM, - # but not visible or reachable via keyboard interaction. + # Basic search field should always be present assert_select 'input#basic-search-main', { count: 1 } - assert_select 'input#advanced-citation', { count: 1 } - assert_select 'input#advanced-contributors', { count: 1 } - assert_select 'input#advanced-fundingInformation', { count: 1 } - assert_select 'input#advanced-identifiers', { count: 1 } - assert_select 'input#advanced-locations', { count: 1 } - assert_select 'input#advanced-subjects', { count: 1 } - assert_select 'input#advanced-title', { count: 1 } - assert_select 'input.source', { minimum: 3 } + + if Flipflop.enabled?(:gdt) + # Please note that this test confirms fields in the DOM - but not whether + # they are visible. Fields in a hidden details panel are still in the DOM, + # but not visible or reachable via keyboard interaction. + assert_select 'input#advanced-citation', { count: 1 } + assert_select 'input#advanced-contributors', { count: 1 } + assert_select 'input#advanced-fundingInformation', { count: 1 } + assert_select 'input#advanced-identifiers', { count: 1 } + assert_select 'input#advanced-locations', { count: 1 } + assert_select 'input#advanced-subjects', { count: 1 } + assert_select 'input#advanced-title', { count: 1 } + assert_select 'input.source', { minimum: 3 } + end end test 'advanced search source checkboxes can be controlled by env' do + skip('Advanced search functionality not implemented in USE UI') get '/' assert_select 'input.source', { minimum: 3 } @@ -88,69 +160,98 @@ def setup assert_equal 'A search term is required.', flash[:error] end - test 'results with valid query displays the query' do - VCR.use_cassette('timdex hallo', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=hallo' - assert_response :success - assert_nil flash[:error] + test 'primo results with valid query displays the query' do + mock_primo_search_success + get '/results?q=hallo' + assert_response :success + assert_nil flash[:error] - assert_select 'input[value=?]', 'hallo' - end + assert_select 'input[value=?]', 'hallo' end - test 'results with valid query shows search form' do - VCR.use_cassette('timdex hallo', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=hallo' - assert_response :success + test 'timdex results with valid query displays the query' do + mock_timdex_search_success + get '/results?q=hallo&tab=timdex' + assert_response :success + assert_nil flash[:error] - assert_select 'form#basic-search', { count: 1 } - end + assert_select 'input[value=?]', 'hallo' end - test 'results with valid query populates search form with query' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=data' - assert_response :success + test 'primo results with valid query shows search form' do + mock_primo_search_success + get '/results?q=hallo' + assert_response :success - assert_select '#basic-search-main[value=data]' - end + assert_select 'form#basic-search', { count: 1 } end - test 'results with valid query has div for hints if any fact panels are enabled' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: 'fakepanel') do - get '/results?q=data' - end + test 'timdex results with valid query shows search form' do + mock_timdex_search_success + get '/results?q=hallo&tab=timdex' + assert_response :success - assert_response :success + assert_select 'form#basic-search', { count: 1 } + end + + test 'primo results with valid query populates search form with query' do + mock_primo_search_success + get '/results?q=data' + assert_response :success + + assert_select '#basic-search-main[value=data]' + end - assert_select '#hint' + test 'timdex results with valid query populates search form with query' do + mock_timdex_search_success + get '/results?q=data&tab=timdex' + assert_response :success + + assert_select '#basic-search-main[value=data]' + end + + test 'primo results with valid query has div for hints if any fact panels are enabled' do + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'fakepanel') do + get '/results?q=data' end + + assert_response :success + assert_select '#hint' end - test 'results with valid query has no div for hints if no fact panels are enabled' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: '') do - get '/results?q=data' - end + test 'timdex results with valid query has div for hints if any fact panels are enabled' do + mock_timdex_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'fakepanel') do + get '/results?q=data&tab=timdex' + end - assert_response :success + assert_response :success + assert_select '#hint' + end - assert_select('#hint', 0) + test 'primo results with valid query has no div for hints if no fact panels are enabled' do + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=data' end + + assert_response :success + assert_select('#hint', 0) + end + + test 'timdex results with valid query has no div for hints if no fact panels are enabled' do + mock_timdex_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=data&tab=timdex' + end + + assert_response :success + assert_select('#hint', 0) end test 'results with valid query has div for filters which is populated' do + skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -162,6 +263,7 @@ def setup end test 'a filter category lists available filters with names and values' do + skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -172,47 +274,55 @@ def setup end end - test 'results with valid query has div for pagination' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=data' - assert_response :success + test 'primo results with valid query has div for pagination' do + mock_primo_search_success + get '/results?q=data' + assert_response :success + assert_select '#pagination' + end - assert_select '#pagination' - end + test 'timdex results with valid query has div for pagination' do + mock_timdex_search_success + get '/results?q=data&tab=timdex' + assert_response :success + assert_select '#pagination' end - test 'results with valid query has div for results which is populated' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=data' - assert_response :success - assert_select '#results' - assert_select '#results .record-title', { minimum: 1 } - end + test 'primo results with valid query has div for results which is populated' do + mock_primo_search_success + get '/results?q=data' + assert_response :success + assert_select '#results' + assert_select '#results .record-title', { minimum: 1 } end - test 'results with valid query include links' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=data' - assert_select '#results .record-title a' do |value| - refute_nil(value.xpath('./@href').text) - end - end + test 'timdex results with valid query has div for results which is populated' do + mock_timdex_search_success + get '/results?q=data&tab=timdex' + assert_response :success + assert_select '#results' + assert_select '#results .record-title', { minimum: 1 } end - test 'results with valid query have query highlights' do - VCR.use_cassette('data basic controller', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - get '/results?q=data' - assert_response :success - assert_select '#results .result-highlights ul li', { minimum: 1 } - end + test 'primo results with valid query include links' do + mock_primo_search_success + get '/results?q=data' + assert_response :success + assert_select '#results .record-title a' + end + + test 'timdex results with valid query include links' do + mock_timdex_search_success + get '/results?q=data&tab=timdex' + assert_response :success + assert_select '#results .record-title a' + end + + test 'timdex results with valid query have query highlights' do + mock_timdex_search_success + get '/results?q=data&tab=timdex' + assert_response :success + assert_select '#results .result-highlights ul li', { minimum: 1 } end test 'highlights partial is not rendered for results with no relevant highlights' do @@ -232,13 +342,13 @@ def setup VCR.use_cassette('timdex no results', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do - get '/results?q=asdfiouwenlasd' + get '/results?q=asdfiouwenlasd&tab=timdex' assert_response :success # Result list contents state "no results" assert_select '#results' assert_select '#results', { count: 1 } - assert_select '#results p', 'No results found for your search' + assert_select '#results .no-results p', 'No results found for your search' # Filter sidebar is not shown assert_select '#filters', { count: 0 } @@ -252,117 +362,90 @@ def setup end test 'searches with ISSN display issn fact card when enabled' do - VCR.use_cassette('timdex 1234-5678', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: 'issn') do - get '/results?q=1234-5678' - end - assert_response :success - - actual_div = assert_select('div[data-content-loader-url-value]') - assert_equal '/issn?issn=1234-5678', - actual_div.attribute('data-content-loader-url-value').value + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'issn') do + get '/results?q=1234-5678' end + assert_response :success + + actual_div = assert_select('div[data-content-loader-url-value]') + assert_equal '/issn?issn=1234-5678', + actual_div.attribute('data-content-loader-url-value').value end test 'searches with ISSN do not display issn fact card when not enabled' do - VCR.use_cassette('timdex 1234-5678', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: '') do - get '/results?q=1234-5678' - end - assert_response :success - - assert_select('div[data-content-loader-url-value].fact-container', 0) + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=1234-5678' end + assert_response :success + + assert_select('div[data-content-loader-url-value].fact-container', 0) end test 'searches with ISBN insert isbn dom element when enabled' do - VCR.use_cassette('timdex 9781857988536', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: 'isbn') do - get '/results?q=9781857988536' - end + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'isbn') do + get '/results?q=9781857988536' + end - assert_response :success + assert_response :success - actual_div = assert_select('div[data-content-loader-url-value]') - assert_equal '/isbn?isbn=9781857988536', - actual_div.attribute('data-content-loader-url-value').value - end + actual_div = assert_select('div[data-content-loader-url-value]') + assert_equal '/isbn?isbn=9781857988536', + actual_div.attribute('data-content-loader-url-value').value end test 'searches with ISBN do not insert isbn dom element when not enabled' do - VCR.use_cassette('timdex 9781857988536', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: '') do - get '/results?q=9781857988536' - end - - assert_response :success - - assert_select('div[data-content-loader-url-value].fact-container', 0) + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=9781857988536' end + + assert_response :success + assert_select('div[data-content-loader-url-value].fact-container', 0) end test 'searches with DOI insert doi dom element when enabled' do - VCR.use_cassette('timdex 10.1038.nphys1170 ', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: 'doi') do - get '/results?q=10.1038/nphys1170 ' - end - assert_response :success - - actual_div = assert_select('div[data-content-loader-url-value]') - assert_equal '/doi?doi=10.1038%2Fnphys1170', - actual_div.attribute('data-content-loader-url-value').value + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'doi') do + get '/results?q=10.1038/nphys1170 ' end + assert_response :success + + actual_div = assert_select('div[data-content-loader-url-value]') + assert_equal '/doi?doi=10.1038%2Fnphys1170', + actual_div.attribute('data-content-loader-url-value').value end test 'searches with DOI do not insert doi dom element when not enabled' do - VCR.use_cassette('timdex 10.1038.nphys1170 ', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: '') do - get '/results?q=10.1038/nphys1170 ' - end - assert_response :success - - assert_select('div[data-content-loader-url-value].fact-container', 0) + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=10.1038/nphys1170 ' end + assert_response :success + assert_select('div[data-content-loader-url-value].fact-container', 0) end test 'searches with pmid insert pmid dom element when enabled' do - VCR.use_cassette('timdex PMID 35649707', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: 'pmid') do - get '/results?q=PMID: 35649707' - end - assert_response :success - - actual_div = assert_select('div[data-content-loader-url-value]') - assert_equal '/pmid?pmid=PMID%3A+35649707', - actual_div.attribute('data-content-loader-url-value').value + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: 'pmid') do + get '/results?q=PMID: 35649707' end + assert_response :success + + actual_div = assert_select('div[data-content-loader-url-value]') + assert_equal '/pmid?pmid=PMID%3A+35649707', + actual_div.attribute('data-content-loader-url-value').value end test 'searches with pmid do not insert pmid dom element when not enabled' do - VCR.use_cassette('timdex PMID 35649707', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do - ClimateControl.modify(FACT_PANELS_ENABLED: '') do - get '/results?q=PMID: 35649707' - end - assert_response :success - - assert_select('div[data-content-loader-url-value].fact-container', 0) + mock_primo_search_success + ClimateControl.modify(FACT_PANELS_ENABLED: '') do + get '/results?q=PMID: 35649707' end + assert_response :success + assert_select('div[data-content-loader-url-value].fact-container', 0) end test 'TACOS intervention is inserted when TACOS enabled' do @@ -391,6 +474,7 @@ def setup # Advanced search behavior test 'advanced search by keyword' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced keyword asdf', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -403,6 +487,7 @@ def setup end test 'can search an advanced field without a keyword search' do + skip('Advanced search not implemented in USE UI') # note, this confirms we only validate param[:q] is present for basic searches VCR.use_cassette('advanced citation asdf', allow_playback_repeats: true, @@ -414,6 +499,7 @@ def setup end test 'advanced search can accept values from all fields' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced all', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -445,6 +531,7 @@ def setup end test 'advanced search form retains values with spaces' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced all spaces', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -479,6 +566,7 @@ def source_filter_count(controller) end test 'advanced search can limit to a single source' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced source limit to one source', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -496,6 +584,7 @@ def source_filter_count(controller) end test 'advanced search defaults to all sources' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced source defaults to all', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -513,6 +602,7 @@ def source_filter_count(controller) end test 'advanced search can limit to multiple sources' do + skip('Advanced search not implemented in USE UI') VCR.use_cassette('advanced source limit to two sources', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -533,6 +623,7 @@ def source_filter_count(controller) end test 'applications can customize the displayed filters via ENV' do + skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -571,6 +662,7 @@ def source_filter_count(controller) end test 'clear all filters button does not appear with zero filters in query' do + skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -581,6 +673,7 @@ def source_filter_count(controller) end test 'clear all filters button does not appear with one filter in query' do + skip('Filters not implemented in USE UI') VCR.use_cassette('filter one', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -595,6 +688,7 @@ def source_filter_count(controller) end test 'clear all filters button appears with more than filter in query' do + skip('Filters not implemented in USE UI') VCR.use_cassette('filter multiple', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do @@ -608,4 +702,57 @@ def source_filter_count(controller) assert_select '.clear-filters', count: 1 end end + + # Tab functionality tests for USE + test 'results defaults to primo tab when no tab parameter provided' do + mock_primo_search_success + + get '/results?q=test' + assert_response :success + assert_select 'a.tab-link.active[href*="tab=primo"]', count: 1 + end + + test 'results respects primo tab parameter' 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 + end + + test 'results respects timdex tab parameter' 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 + end + + test 'results shows tab navigation when gdt is disabled' do + mock_primo_search_success + + get '/results?q=test' + 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 + end + + test 'results handles primo search errors gracefully' do + PrimoSearch.expects(:new).raises(StandardError.new('API Error')) + + get '/results?q=test&tab=primo' + assert_response :success + assert_select '.alert', count: 1 + assert_select '.alert', text: /API Error/ + end + + test 'results uses simplified search summary for USE app' do + mock_primo_search_success + + get '/results?q=test' + assert_response :success + assert_select 'aside.search-summary', count: 1 + assert_select 'aside.search-summary', text: /You searched for: test/ + end end diff --git a/test/integration/error_resilience_test.rb b/test/integration/error_resilience_test.rb index d157f796..319238ec 100644 --- a/test/integration/error_resilience_test.rb +++ b/test/integration/error_resilience_test.rb @@ -7,7 +7,7 @@ class ErrorResilienceTest < ActionDispatch::IntegrationTest VCR.use_cassette('timdex error', allow_playback_repeats: true, match_requests_on: %i[method uri body]) do - get '/results?q=poverty' + get '/results?q=poverty&tab=timdex' assert_response :success assert_select('.alert', true) assert_select('.alert') do |value|