Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,24 @@ class ApplicationController < ActionController::Base
# We set this in a session cookie to persist user preference across searches.
# Clicking on a different tab will update the cookie.
def set_active_tab
@active_tab = if Feature.enabled?(:geodata)
# Determine which tab to load - default to primo unless gdt is enabled
'geodata'
elsif params[:tab].present?
# If params[:tab] is set, use it and set session
# 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]
elsif cookies[:last_tab].present?
# Otherwise, check for last used tab in session
elsif cookies[:last_tab].present? && valid_tab?(cookies[:last_tab])
# Otherwise, check for last used tab in session if valid
cookies[:last_tab]
else
# Default behavior when no tab is specified in params or session
cookies[:last_tab] = 'primo'
cookies[:last_tab] = 'all'
end
end

private

def valid_tab?(tab)
%w[primo timdex all].include?(tab)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively to removing the first condition in the active_tab logic, we should instead set geodata as a valid tab. I think in real-world scenarios that is fine but when changing your local or a pr build between geodata and normal you may end up in wonky states where the cookie says to load an invalid tab?

I think I'd either remove the geodata logic from active_tab or keep it and add which tabs are valid here in each mode with a feature flag. I'm happy to chat if it would be helpful to work through the edge cases we are trying to support in real time.

end
end
99 changes: 76 additions & 23 deletions app/controllers/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,21 @@ def results

@enhanced_query = Enhancer.new(params).enhanced_query

# Load GeoData results if applicable
if Feature.enabled?(:geodata)
load_geodata_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 'geodata'
load_gdt_results
render 'results_geo'
when 'all'
load_all_results
end
end

Expand All @@ -50,7 +56,7 @@ def sleep_if_too_fast
sleep(1 - duration)
end

def load_gdt_results
def load_geodata_results
query = QueryBuilder.new(@enhanced_query).query

response = query_timdex(query)
Expand All @@ -66,49 +72,96 @@ def load_gdt_results
end

def load_primo_results
data = fetch_primo_data
@results = data[:results]
@pagination = data[:pagination]
@errors = data[:errors]
@show_primo_continuation = data[:show_continuation]
end

def load_timdex_results
data = fetch_timdex_data
@results = data[:results]
@pagination = data[:pagination]
@errors = data[:errors]
end

def load_all_results
# Parallel fetching from both APIs
primo_thread = Thread.new { fetch_primo_data }
timdex_thread = Thread.new { fetch_timdex_data }

# Wait for both threads to complete
primo_data = primo_thread.value
timdex_data = timdex_thread.value

# Collect any errors from either API
all_errors = []
all_errors.concat(primo_data[:errors]) if primo_data[:errors]
all_errors.concat(timdex_data[:errors]) if timdex_data[:errors]
@errors = all_errors.any? ? all_errors : nil

# Zipper merge results from both APIs
primo_results = primo_data[:results] || []
timdex_results = timdex_data[:results] || []
@results = primo_results.zip(timdex_results).flatten.compact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


# 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

def fetch_primo_data
current_page = @enhanced_query[:page] || 1
per_page = @enhanced_query[:per_page] || 20
offset = (current_page - 1) * per_page

# Check if we're beyond Primo API limits before making the request.
if offset >= Analyzer::PRIMO_MAX_OFFSET
@show_primo_continuation = true
return
return { results: [], pagination: {}, errors: nil, show_continuation: true }
end

primo_response = query_primo
@results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize
results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize
pagination = Analyzer.new(@enhanced_query, primo_response, :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
# seemingly for no reason (well below the recommended offset of 2,000). While the bug also
# exists in Primo UI, sending users there seems like the best we can do.
if @results.empty?
show_continuation = false
errors = nil

if results.empty?
docs = primo_response['docs'] if primo_response.is_a?(Hash)
if docs.nil? || docs.empty?
# Only show continuation for pagination scenarios (page > 1), not for searches with no results
@show_primo_continuation = true if current_page > 1
show_continuation = true if current_page > 1
else
@errors = [{ 'message' => 'No more results available at this page number.' }]
errors = [{ 'message' => 'No more results available at this page number.' }]
end
end

# Use Analyzer for consistent pagination across all search types
@pagination = Analyzer.new(@enhanced_query, primo_response, :primo).pagination
{ results: results, pagination: pagination, errors: errors, show_continuation: show_continuation }
rescue StandardError => e
@errors = handle_primo_errors(e)
{ results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false }
end

def load_timdex_results
def fetch_timdex_data
query = QueryBuilder.new(@enhanced_query).query
response = query_timdex(query)

@errors = extract_errors(response)
return unless @errors.nil?

@pagination = Analyzer.new(@enhanced_query, response, :timdex).pagination
raw_results = extract_results(response)
@results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize
errors = extract_errors(response)

if errors.nil?
pagination = Analyzer.new(@enhanced_query, response, :timdex).pagination
raw_results = extract_results(response)
results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize
{ results: results, pagination: pagination, errors: nil }
else
{ results: [], pagination: {}, errors: errors }
end
end

def active_filters
Expand All @@ -121,9 +174,9 @@ def query_timdex(query)

# Builder hands off to wrapper which returns raw results here.
Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do
raw = if @active_tab == 'geodata'
raw = if Feature.enabled?(:geodata)
execute_geospatial_query(query)
elsif @active_tab == 'timdex'
elsif @active_tab == 'timdex' || @active_tab == 'all'
TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query)
end

Expand Down
2 changes: 1 addition & 1 deletion app/helpers/filter_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def nice_labels
}
end

def gdt_sources(value, category)
def geodata_sources(value, category)
return value if category != :sourceFilter

return 'Non-MIT institutions' if value == 'opengeometadata gis resources'
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/search_form.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function updateKeywordPlaceholder() {
}
}

// Add event listeners for all panels in the DOM. For GDT, this is currently both geospatial panels and the advanced
// Add event listeners for all panels in the DOM. For GeoData, this is currently both geospatial panels and the advanced
// panel. In all other TIMDEX UI apps, it's just the advanced panel.
if (Array.from(allPanels).includes(geoboxPanel && geodistancePanel)) {
document.getElementById('geobox-summary').addEventListener('click', () => {
Expand Down
8 changes: 4 additions & 4 deletions app/models/feature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
#
# @example Setting Flags in Environment
# # In local ENV or Heroku config:
# FEATURE_GEODATA=true # Enables the GDT feature
# FEATURE_GEODATA=true # Enables the GeoData feature
# FEATURE_BOOLEAN_PICKER=true # Enables the boolean picker feature
#
# # Any non-true value or not setting ENV does not enable the feature
# FEATURE_GEODATA=false # Does not enable the GDT feature
# FEATURE_GEODATA=1 # Does not enable the GDT feature
# FEATURE_GEODATA=on # Does not enable the GDT feature
# FEATURE_GEODATA=false # Does not enable the GeoData feature
# FEATURE_GEODATA=1 # Does not enable the GeoData feature
# FEATURE_GEODATA=on # Does not enable the GeoData feature
#
# @example Usage in Different Contexts
# # In models/controllers:
Expand Down
2 changes: 1 addition & 1 deletion app/views/record/_record_geo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
</ul>
<% end %>

<!-- The publishers field also includes location and date subfields, but these are unused in GDT. -->
<!-- The publishers field also includes location and date subfields, but these are unused in GeoData. -->
<% if @record['publishers'].present? %>
<h3 class="section-title">Publishers</h3>
<ul>
Expand Down
2 changes: 1 addition & 1 deletion app/views/search/_filter.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<span class="sr">Apply filter:</span>
<% end %>
<% if Feature.enabled?(:geodata) %>
<span class="name"><%= gdt_sources(term['key'], category) %></span>
<span class="name"><%= geodata_sources(term['key'], category) %></span>
<% else %>
<span class="name"><%= term['key'] %></span>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/search/_search_summary_geo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
href="<%= results_path(remove_filter(@enhanced_query, filter.keys[0], filter.values[0])) %>">
<%= "#{nice_labels[filter.keys[0]] || filter.keys[0]}:" %>
<% if Feature.enabled?(:geodata) %>
<%= "#{gdt_sources(filter.values[0], filter.keys[0])}" %>
<%= "#{geodata_sources(filter.values[0], filter.keys[0])}" %>
<% else %>
<%= "#{filter.values[0]}" %>
<% end %>
Expand Down
1 change: 1 addition & 0 deletions app/views/search/_source_tabs.html.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<!-- Tab Navigation -->
<nav id="tabs" class="tab-navigation" aria-label="Result type navigation">
<ul>
<li><%= link_to_tab("All") %></li>
<li><%= link_to_tab("Primo") %></li>
<li><%= link_to_tab("TIMDEX") %></li>
</ul>
Expand Down
8 changes: 8 additions & 0 deletions app/views/search/results.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
<%= 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' %>
<%= render(partial: 'search/result_primo', locals: { result: result }) %>
<% else %>
<%= render(partial: 'search/result', locals: { result: result }) %>
<% end %>
<% end %>
<% end %>
</ol>
<% elsif @errors.blank? %>
Expand Down
Loading