Skip to content

Add search summary panel and full filter support #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Add search summary panel and full filter support #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 33 additions & 0 deletions app/assets/stylesheets/partials/_panels.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,36 @@
}
}
}

.filter-summary {
color: $black;
background-color: $gray-l3;
margin-top: 0;
border: 0;
padding: 2rem;
padding-top: 1.5rem;
display: flex;
.hd-filter-summary {
margin-right: 2rem;
padding-top: 0.3rem;
}
a {
font-size: $fs-small;
text-decoration: none;
padding: .5rem;
&::before {
font-family: FontAwesome;
content: '\f00d';
padding-right: .25rem;
}
&:hover,
&:focus {
color: $white;
background-color: $black;
}
margin-right: 2rem;
}
@media (max-width: $bp-screen-md) {
display: block;
}
}
2 changes: 1 addition & 1 deletion app/assets/stylesheets/partials/_search.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/* basic search bar */
.basic-search {
background-color: #989898;
margin-bottom: 1rem;
margin-bottom: 0rem;
padding: 1.6rem 2rem;

details {
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/search_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ def extract_filters(response)
aggs = response&.data&.search&.to_h&.dig('aggregations')
return if aggs.blank?

aggs.select { |_, agg_values| agg_values.present? }
# We use aggregations to determine which terms can be filtered. However, agg names do not include 'filter', whereas
# our filter fields do (e.g., 'source' vs 'sourceFilter'). Because of this mismatch, we need to modify the
# aggregation key names before collecting them as filters, so that when a filter is applied, it searches the
# correct field name.
aggs.select { |_, agg_values| agg_values.present? }.transform_keys { |key| (key.dup << 'Filter').to_sym }
Copy link
Member

Choose a reason for hiding this comment

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

Does this feel like a workaround to a deficiency in our API? Or is this something only our UI is likely to encounter?

It feels like we should be returning our aggregations using the same terminology as we use to apply them. So instead of returning source we should return that aggregation as sourceFilter so it is clearer how to use the data. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. I hate this code, and manipulating the response this much in the UI certainly feels like a workaround, but I don't know if I'm convinced we should change our aggregation names. Since they're not filter/filters aggregations, naming them as such might be confusing to other consumers. Less confusing than having different names for the corresponding aggs, though? I'm not sure.

end

def extract_results(response)
Expand Down
16 changes: 13 additions & 3 deletions app/helpers/filter_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def add_filter(query, filter, term)
# seems like the best solution as each record only has a single source
# in the data so there will never be a case to apply multiple in an AND
# which is all we support in filter application.
if new_query[filter].present? && filter != 'source'
if new_query[filter].present? && filter != :sourceFilter
new_query[filter] << term
new_query[filter].uniq!
else
Expand All @@ -21,8 +21,8 @@ def add_filter(query, filter, term)

def nice_labels
{
'contentType' => 'Content types',
'source' => 'Sources'
contentTypeFilter: 'Content type',
sourceFilter: 'Source'
}
end

Expand All @@ -44,4 +44,14 @@ def filter_applied?(terms, term)

terms.include?(term)
end

def applied_filters
filters = []
@enhanced_query.map do |param, values|
next unless param.to_s.include? 'Filter'

values.each { |value| filters << { param => value } }
end
filters
end
end
5 changes: 2 additions & 3 deletions app/helpers/form_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ module FormHelper
def source_checkbox(source, params)
"<div class='field-subitem'>
<label class='field-checkbox'>
<input type='checkbox' value='#{source.downcase}' name='source[]' class='source'#{if params[:source]&.include?(source.downcase)
' checked'
end}>
<input type='checkbox' value='#{source.downcase}' name='sourceFilter[]'
class='source'#{' checked' if params[:sourceFilter]&.include?(source.downcase) }>
#{source}
</label>
</div>".html_safe
Expand Down
2 changes: 1 addition & 1 deletion app/models/enhancer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class Enhancer
attr_accessor :enhanced_query

QUERY_PARAMS = %i[q citation contentType contributors fundingInformation identifiers locations subjects title].freeze
FILTER_PARAMS = [:source].freeze
FILTER_PARAMS = %i[sourceFilter contentTypeFilter].freeze
# accepts all params as each enhancer may require different data
def initialize(params)
@enhanced_query = {}
Expand Down
8 changes: 4 additions & 4 deletions app/models/query_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class QueryBuilder

RESULTS_PER_PAGE = 20
QUERY_PARAMS = %w[q citation contributors fundingInformation identifiers locations subjects title].freeze
FILTER_PARAMS = %i[source contentType].freeze
FILTER_PARAMS = %i[sourceFilter contentTypeFilter].freeze

def initialize(enhanced_query)
@query = {}
Expand All @@ -29,8 +29,8 @@ def extract_query(enhanced_query)
end

def extract_filters(enhanced_query)
# NOTE: ui and backend naming are not aligned so we can't loop here. we should fix in UI
@query['sourceFilter'] = enhanced_query[:source]
@query['contentType'] = enhanced_query[:contentType]
FILTER_PARAMS.each do |qp|
@query[qp] = enhanced_query[qp]
end
end
end
4 changes: 2 additions & 2 deletions app/models/timdex_search.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class TimdexSearch < TimdexBase
$sourceFilter: [String!]
$index: String
$from: String
$contentType: [String!]
$contentTypeFilter: [String!]
) {
search(
searchterm: $q
Expand All @@ -29,7 +29,7 @@ class TimdexSearch < TimdexBase
sourceFilter: $sourceFilter
index: $index
from: $from
contentTypeFilter: $contentType
contentTypeFilter: $contentTypeFilter
) {
hits
records {
Expand Down
2 changes: 1 addition & 1 deletion app/views/search/_filter.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<% return if values.empty? %>
<% return if values.blank? %>

<div class="category">
<button class="filter-label <%= 'expanded' if @enhanced_query[category.to_sym].present? || first == true %>"
Expand Down
15 changes: 15 additions & 0 deletions app/views/search/_search_summary.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<% return unless applied_filters.any? %>

<div class="filter-summary">
<h2 class="hd-filter-summary hd-5">Applied filters: </h2>
<ul class="list-filter-summary list-inline">
<% applied_filters.each do |filter| %>
<li>
<a href="<%= results_path(remove_filter(@enhanced_query, filter.keys[0], filter.values[0])) %>">
<%= "#{nice_labels[filter.keys[0]] || filter.keys[0]}: #{filter.values[0]}" %>
<span class="sr">Remove applied filter?</span>
</a>
</li>
<% end %>
</ul>
</div>
41 changes: 3 additions & 38 deletions app/views/search/results.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,8 @@
<%= content_for(:title, "Search Results | MIT Libraries") %>

<div class="space-wrap">

<p>Showing results for:
<ul>
<% if @enhanced_query[:q].present? %>
<li>Keyword anywhere: <%= @enhanced_query[:q] %></li>
<% end %>
<% if @enhanced_query[:citation].present? %>
<li>Citation: <%= @enhanced_query[:citation] %></li>
<% end %>
<% if @enhanced_query[:contentType].present? %>
<li>Content type: <%= @enhanced_query[:contentType] %></li>
<% end %>
<% if @enhanced_query[:contributors].present? %>
<li>Contributors: <%= @enhanced_query[:contributors] %></li>
<% end %>
<% if @enhanced_query[:fundingInformation].present? %>
<li>Funders: <%= @enhanced_query[:fundingInformation] %></li>
<% end %>
<% if @enhanced_query[:identifiers].present? %>
<li>Identifiers: <%= @enhanced_query[:identifiers] %></li>
<% end %>
<% if @enhanced_query[:locations].present? %>
<li>Locations: <%= @enhanced_query[:locations] %></li>
<% end %>
<% if @enhanced_query[:subjects].present? %>
<li>Subjects: <%= @enhanced_query[:subjects] %></li>
<% end %>
<% if @enhanced_query[:title].present? %>
<li>Title: <%= @enhanced_query[:title] %></li>
<% end %>
<% if @enhanced_query[:source].present? %>
<li>Source: <%= @enhanced_query[:source] %></li>
<% end %>
</ul>
</p>

<%= render partial: "form" %>
<%= render partial: "search_summary" %>

<div id="hint" aria-live="polite">
<%= render(partial: 'search/issn') %>
Expand All @@ -56,9 +21,9 @@
<h3 class="hd-4"><em><%= results_summary(@pagination[:hits]) %></em></h3>
<% @filters&.each_with_index do |(category, values), index| %>
<% if index == 0 %>
<%= render(partial: 'search/filter', locals: {category: category, values: values, first: true }) %>
<%= render(partial: 'search/filter', locals: { category: category, values: values, first: true }) %>
<% else %>
<%= render(partial: 'search/filter', locals: {category: category, values: values, first: false }) %>
<%= render(partial: 'search/filter', locals: { category: category, values: values, first: false }) %>
<% end %>
<% end %>
</div>
Expand Down
14 changes: 8 additions & 6 deletions test/controllers/search_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class SearchControllerTest < ActionDispatch::IntegrationTest
assert_response :success
assert_nil flash[:error]

assert_select 'li', 'Keyword anywhere: hallo'
assert_select 'input[value=?]', 'hallo'
end
end

Expand Down Expand Up @@ -272,6 +272,8 @@ class SearchControllerTest < ActionDispatch::IntegrationTest

# Advanced search behavior
test 'advanced search by keyword' do
skip("We no longer display a list of search terms in the UI; leaving this in in case we decide to reintroduce" \
Copy link
Member

Choose a reason for hiding this comment

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

I know you brought this to the team last week, but after seeing in action, I'm fairly hesitant to remove this feature as it reads very weird the way it is implemented as it feels like it sits halfway between "these are all the things you searched for" and "these are the filters that are applied but please look at the form above for the rest of the values you searched for". i.e. the words say one thing but the implementation is the other... which is awkward.

My preference would be to label it in the way it is currently functioning (Applied filters: ...) as that feels like the easiest solution for now even if it doesn't match the requested wording. The alternative would be to add all of the various searchable bits to this new panel which honestly feels like a better longterm direction.

If there is a ticket to resolve this distinction after talking with UX when they are back from vacation, I could probably get over this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd strongly prefer changing the label to Applied filters. In my opinion, the "You searched for" list is redundant with the expanded form. (And, honestly, I'm not sure what these tests are confirming, except that these fields show up in the enhanced query.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, sprint planning reminded me that I created a ticket to figure out what we want to do with advanced search terms: https://mitlibraries.atlassian.net/browse/GDT-196

"that feature soon.")
VCR.use_cassette('advanced keyword asdf',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
Expand All @@ -291,12 +293,12 @@ class SearchControllerTest < ActionDispatch::IntegrationTest
get '/results?citation=asdf&advanced=true'
assert_response :success
assert_nil flash[:error]

assert_select 'li', 'Citation: asdf'
end
end

test 'advanced search can accept values from all fields' do
skip("We no longer display a list of search terms in the UI; leaving this in in case we decide to reintroduce" \
"that feature soon.")
VCR.use_cassette('advanced all',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
Expand Down Expand Up @@ -359,7 +361,7 @@ class SearchControllerTest < ActionDispatch::IntegrationTest
end

def source_filter_count(controller)
controller.view_context.assigns['filters']['source'].count
controller.view_context.assigns['filters'][:sourceFilter].count
end

test 'advanced search can limit to a single source' do
Expand All @@ -369,7 +371,7 @@ def source_filter_count(controller)
query = {
q: 'data',
advanced: 'true',
source: ['Woods Hole Open Access Server']
sourceFilter: ['Woods Hole Open Access Server']
}.to_query
get "/results?#{query}"
assert_response :success
Expand Down Expand Up @@ -406,7 +408,7 @@ def source_filter_count(controller)
query = {
q: 'data',
advanced: 'true',
source: ['Abdul Latif Jameel Poverty Action Lab Dataverse', 'Woods Hole Open Access Server']
sourceFilter: ['Abdul Latif Jameel Poverty Action Lab Dataverse', 'Woods Hole Open Access Server']
}.to_query
get "/results?#{query}"
assert_response :success
Expand Down
Loading