Skip to content

Commit

Permalink
Implement location-based search
Browse files Browse the repository at this point in the history
Why these changes are being introduced:

The GeoData app needs to allow for geospatial searching.

Relevant ticket(s):

* [GDT-136](https://mitlibraries.atlassian.net/browse/GDT-136)

How this addresses that need:

This adds form elements for the geospatial fields we’ve added to TIMDEX: geobox and geodistance. Because these fields
are relatively complex and all their arguments are non-nullable, each of them has its own drop-down that toggles a
boolean param (`:geobox` and `:geodistance`, respectively) to pass the field’s arguments into the TIMDEX query.

Certain client-side form validations have been added (e.g., range for lat/long entries), as well as corresponding
controller validations in case a user bypasses the form.

In order to achieve the multiple query permutations (no geo fields, both geo fields, one of either geo field),
additional query objects have been added. This is based on the recommendation of the graphql-client gem, which
suggests using statically declared heredocs as inputs. However, that gem also recommends making queries out of several
smaller heredocs, using GraphQL conveniences like fragments, so our implementation may not be exactly what the gem
maintainers intended.

Side effects of this change:

* Introducing an additional monolithic query object for each permutation doesn’t feel scalable or OO. We likely don’t
have to time to investigate this now, but it would be interesting to consider alternative methods to constructing
queries, and possibly also different GraphQL client gems.
* The controller validations are an important failsafe, but they are not especially friendly. We may want to consider
additional client-side validations to reduce the likelihood of encountering the controller validations in the wild.
* Writing this feature revealed that the Enhancer and Query Builder models are functionally identical. This was
intentional to prepare the app for additional query enhancements. As we’ve now abstracted that to TACOS, we should
consider at some point whether we still need both of these models.
* The form partial and corresponding JS are getting unwieldy. Refactoring these would be a good maintenance activity.
Further, the interactive elements are becoming complex enough that E2E testing is worth considering.
* The labels for the new form elements are grotesquely long, but they also need to convey a lot of information. I’m
hoping that UXWS can help with this.
* There are a lot of geo tests, so rather than switching the test strategy in each test, I isolated them to their
own class to set the test strategy in a minitest lifecycle hook.
* Most cassettes have been regenerated due to schema updates.

Isolate test strategy to geo tests
  • Loading branch information
jazairi committed Mar 4, 2024
1 parent 1893e51 commit bc14377
Show file tree
Hide file tree
Showing 43 changed files with 2,397 additions and 283 deletions.
1 change: 1 addition & 0 deletions .env.test
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
TIMDEX_HOST=FAKE_TIMDEX_HOST
TIMDEX_GRAPHQL=https://FAKE_TIMDEX_HOST/graphql
TIMDEX_INDEX=FAKE_TIMDEX_INDEX
GDT=false
12 changes: 11 additions & 1 deletion app/assets/stylesheets/partials/_search.scss
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@
summary {
pointer: arrow;

#advanced-search-label {
#advanced-search-label,
#geobox-search-label,
#geodistance-search-label {
&::before {
font-family: FontAwesome;
margin-right: 1rem;
Expand All @@ -64,6 +66,14 @@
}
}

#geobox-search-panel,
#geodistance-search-panel {
label::after {
color: $red-muted;
content: '*';
}
}

.basic-search-submit {
@media (min-width: $bp-screen-sm) {
display: inline-block;
Expand Down
88 changes: 87 additions & 1 deletion app/controllers/search_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
class SearchController < ApplicationController
before_action :validate_q!, only: %i[results]

if Flipflop.enabled?(:gdt)
before_action :validate_geobox_presence!, only: %i[results]
before_action :validate_geobox_range!, only: %i[results]
before_action :validate_geodistance_presence!, only: %i[results]
before_action :validate_geodistance_range!, only: %i[results]
before_action :validate_geobox_values!, only: %i[results]
end

def results
# hand off to Enhancer chain
@enhanced_query = Enhancer.new(params).enhanced_query
Expand All @@ -9,7 +17,11 @@ def results
query = QueryBuilder.new(@enhanced_query).query

# builder hands off to wrapper which returns raw results here
response = TimdexBase::Client.query(TimdexSearch::Query, variables: query)
response = if Flipflop.enabled?(:gdt)
execute_geospatial_query(query)
else
TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query)
end

# Handle errors
@errors = extract_errors(response)
Expand All @@ -25,6 +37,18 @@ def results

private

def execute_geospatial_query(query)
if query['geobox'] == 'true' && query[:geodistance] == 'true'
TimdexBase::Client.query(TimdexSearch::AllQuery, variables: query)
elsif query['geobox'] == 'true'
TimdexBase::Client.query(TimdexSearch::GeoboxQuery, variables: query)
elsif query['geodistance'] == 'true'
TimdexBase::Client.query(TimdexSearch::GeodistanceQuery, variables: query)
else
TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query)
end
end

def extract_errors(response)
response&.errors&.details&.to_h&.dig('data')
end
Expand All @@ -46,9 +70,71 @@ def extract_results(response)

def validate_q!
return if params[:advanced]&.strip.present?
return if params[:geobox]&.strip.present?
return if params[:geodistance]&.strip.present?
return if params[:q]&.strip.present?

flash[:error] = 'A search term is required.'
redirect_to root_url
end

def validate_geodistance_presence!
return unless params[:geodistance]&.strip == 'true'

geodistance_params = [params[:geodistanceLatitude]&.strip, params[:geodistanceLongitude]&.strip,
params[:geodistanceDistance]&.strip]
return if geodistance_params.all?(&:present?)

flash[:error] = 'All geospatial distance fields are required.'
redirect_to root_url
end

def validate_geobox_presence!
return unless params[:geobox]&.strip == 'true'

geobox_params = [params[:geoboxMinLatitude]&.strip, params[:geoboxMinLongitude]&.strip,
params[:geoboxMaxLatitude]&.strip, params[:geoboxMaxLongitude]&.strip]
return if geobox_params.all?(&:present?)

flash[:error] = 'All bounding box fields are required.'
redirect_to root_url
end

def validate_geodistance_range!
return unless params[:geodistance]&.strip == 'true'

lat = params[:geodistanceLatitude]&.strip.to_f
long = params[:geodistanceLongitude]&.strip.to_f
return if lat.between?(-90.0, 90.0)
return if long.between?(-180.0, 180.0)

flash[:error] = 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.'
redirect_to root_url
end

def validate_geobox_range!
return unless params[:geobox]&.strip == 'true'

geobox_lat = [params[:geoboxMinLatitude]&.strip.to_f, params[:geoboxMaxLatitude]&.strip.to_f]
geobox_long = [params[:geoboxMinLongitude]&.strip.to_f, params[:geoboxMaxLongitude]&.strip.to_f]
return if geobox_lat.all? { |lat| lat.between?(-90.0, 90.0) }
return if geobox_long.all? { |long| long.between?(-180.0, 180.0) }

flash[:error] = 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.'
redirect_to root_url
end

def validate_geobox_values!
return unless params[:geobox]&.strip == 'true'

geobox_params = [params[:geoboxMinLatitude]&.strip.to_f, params[:geoboxMinLongitude]&.strip.to_f,
params[:geoboxMaxLatitude]&.strip.to_f, params[:geoboxMaxLongitude]&.strip.to_f]

# Confirm that min values are lower than max values
return if geobox_params.any?(&:blank?)
return if geobox_params[0] < geobox_params[2] && geobox_params[1] < geobox_params[3]

flash[:error] = 'Bounding box minimum lat/long cannot exceed maximum lat/long.'
redirect_to root_url
end
end
117 changes: 110 additions & 7 deletions app/javascript/search_form.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,139 @@
function disableAdvanced() {
advanced_field.setAttribute('value', '');
keyword_field.setAttribute('aria-required', true);
if (geobox_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', true);
keyword_field.setAttribute('placeholder', 'Enter your search');
}
[...details_panel.getElementsByClassName('field')].forEach(
field => field.value = ''
);
keyword_field.setAttribute('placeholder', 'Enter your search');
advanced_label.classList = 'closed';
advanced_label.innerText = 'Advanced search';
};

function enableAdvanced() {
advanced_field.setAttribute('value', 'true');
keyword_field.setAttribute('aria-required', false);
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
if (geobox_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', false);
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
}
advanced_label.classList = 'open';
advanced_label.innerText = 'Close advanced search';
};

function disableGeobox() {
if (advanced_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', true);
keyword_field.setAttribute('placeholder', 'Enter your search');
}
geobox_field.setAttribute('value', '');
[...geobox_details_panel.getElementsByClassName('field')].forEach(function(field) {
field.value = '';
field.classList.toggle('required');
field.toggleAttribute('required');
field.setAttribute('aria-required', false);
});
geobox_label.classList = 'closed';
geobox_label.innerText = 'Bounding box search';
};

function enableGeobox() {
if (advanced_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', false);
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
}
geobox_field.setAttribute('value', 'true');
[...geobox_details_panel.getElementsByClassName('field')].forEach(function(field) {
field.value = '';
field.classList.toggle('required');
field.toggleAttribute('required');
field.setAttribute('aria-required', true);
});
geobox_label.classList = 'open';
geobox_label.innerText = 'Close bounding box search';
};

function disableGeodistance() {
if (advanced_label.classList.contains('closed') && geobox_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', true);
keyword_field.setAttribute('placeholder', 'Enter your search');
}
geodistance_field.setAttribute('value', '');
[...geodistance_details_panel.getElementsByClassName('field')].forEach(function(field) {
field.value = '';
field.classList.toggle('required');
field.toggleAttribute('required');
field.setAttribute('aria-required', false);
});
geodistance_label.classList = 'closed';
geodistance_label.innerText = 'Geospatial distance search';
};

function enableGeodistance() {
if (advanced_label.classList.contains('closed') && geobox_label.classList.contains('closed')) {
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
keyword_field.setAttribute('aria-required', false);
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
}
geodistance_field.setAttribute('value', 'true');
[...geodistance_details_panel.getElementsByClassName('field')].forEach(function(field) {
field.value = '';
field.classList.toggle('required');
field.toggleAttribute('required');
field.setAttribute('aria-required', true);
});
geodistance_label.classList = 'open';
geodistance_label.innerText = 'Close geospatial distance search';
};


var advanced_field = document.getElementById('advanced-search-field');
var advanced_label = document.getElementById('advanced-search-label');
var advanced_toggle = document.querySelector('summary');
var advanced_toggle = document.getElementById('advanced-summary');
var details_panel = document.getElementById('advanced-search-panel');
var keyword_field = document.getElementById('basic-search-main');
var geobox_field = document.getElementById('geobox-search-field');
var geobox_label = document.getElementById('geobox-search-label');
var geobox_toggle = document.getElementById('geobox-summary');
var geobox_details_panel = document.getElementById('geobox-search-panel');
var geodistance_field = document.getElementById('geodistance-search-field');
var geodistance_label = document.getElementById('geodistance-search-label');
var geodistance_toggle = document.getElementById('geodistance-summary');
var geodistance_details_panel = document.getElementById('geodistance-search-panel');

geobox_toggle.addEventListener('click', event => {
if (geobox_details_panel.attributes.hasOwnProperty('open')) {
disableGeobox();
} else {
enableGeobox();
}
});

geodistance_toggle.addEventListener('click', event => {
if (geodistance_details_panel.attributes.hasOwnProperty('open')) {
disableGeodistance();
} else {
enableGeodistance();
}
});

advanced_toggle.addEventListener('click', event => {
if (details_panel.attributes.hasOwnProperty('open')) {
disableAdvanced();
} else {
enableAdvanced();
}
keyword_field.toggleAttribute('required');
keyword_field.classList.toggle('required');
});

console.log('search_form.js loaded');
18 changes: 17 additions & 1 deletion app/models/enhancer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,22 @@ class Enhancer
attr_accessor :enhanced_query

QUERY_PARAMS = %i[q citation contentType contributors fundingInformation identifiers locations subjects title].freeze
GEO_PARAMS = %i[geoboxMinLongitude geoboxMinLatitude geoboxMaxLongitude geoboxMaxLatitude geodistanceLatitude
geodistanceLongitude geodistanceDistance].freeze
FILTER_PARAMS = %i[sourceFilter contentTypeFilter].freeze
# accepts all params as each enhancer may require different data
def initialize(params)
@enhanced_query = {}
@enhanced_query[:page] = calculate_page(params[:page].to_i)
@enhanced_query[:advanced] = true if params[:advanced].present?
@enhanced_query[:advanced] = 'true' if params[:advanced].present?

if Flipflop.enabled?(:gdt)
@enhanced_query[:geobox] = 'true' if params[:geobox] == 'true'
@enhanced_query[:geodistance] = 'true' if params[:geodistance] == 'true'
end

extract_query(params)
extract_geosearch(params)
extract_filters(params)
patterns(params) if params[:q]
end
Expand All @@ -26,6 +34,14 @@ def extract_query(params)
end
end

def extract_geosearch(params)
return unless Flipflop.enabled?(:gdt)

GEO_PARAMS.each do |gp|
@enhanced_query[gp] = params[gp] if params[gp].present?
end
end

def extract_filters(params)
FILTER_PARAMS.each do |fp|
@enhanced_query[fp] = params[fp] if params[fp].present?
Expand Down
26 changes: 26 additions & 0 deletions app/models/query_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,21 @@ class QueryBuilder

RESULTS_PER_PAGE = 20
QUERY_PARAMS = %w[q citation contributors fundingInformation identifiers locations subjects title].freeze
GEO_PARAMS = %w[geoboxMinLongitude geoboxMinLatitude geoboxMaxLongitude geoboxMaxLatitude geodistanceLatitude
geodistanceLongitude geodistanceDistance].freeze
FILTER_PARAMS = %i[sourceFilter contentTypeFilter].freeze

def initialize(enhanced_query)
@query = {}
@query['from'] = calculate_from(enhanced_query[:page])

if Flipflop.enabled?(:gdt)
@query['geobox'] = 'true' if enhanced_query[:geobox] == 'true'
@query['geodistance'] = 'true' if enhanced_query[:geodistance] == 'true'
end

extract_query(enhanced_query)
extract_geosearch(enhanced_query)
extract_filters(enhanced_query)
@query['index'] = ENV.fetch('TIMDEX_INDEX', nil)
@query.compact!
Expand All @@ -28,9 +37,26 @@ def extract_query(enhanced_query)
end
end

def extract_geosearch(enhanced_query)
return unless Flipflop.enabled?(:gdt)

GEO_PARAMS.each do |gp|
if coerce_to_float?(gp)
@query[gp] = enhanced_query[gp.to_sym]&.strip.to_f unless enhanced_query[gp.to_sym].blank?
else
@query[gp] = enhanced_query[gp.to_sym]&.strip
end
end
end

def extract_filters(enhanced_query)
FILTER_PARAMS.each do |qp|
@query[qp] = enhanced_query[qp]
end
end

# The GraphQL API requires that lat/long in geospatial fields be floats
def coerce_to_float?(gp)
gp.to_s.include?('Longitude') || gp.to_s.include?('Latitude')
end
end
Loading

0 comments on commit bc14377

Please sign in to comment.