From 02db6ad47e1aabacd91a6d35bd3944cf56a33496 Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Wed, 14 Feb 2024 17:53:16 -0500 Subject: [PATCH] Add search summary panel and full filter support Why these changes are being introduced: It would be helpful if users could see what filters they've applied to a search directly below the search bar, which would also provide another path to remove filters. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/GDT-142 https://mitlibraries.atlassian.net/browse/GDT-196 How this addresses that need: This provides the requested panel. We had been calling this a "filter removal" panel, but I ended up naming the partial `search_summary` to avoid any confusion with the filter sidebar. However, the currently displayed data has the `filter-removal` class, because it's possible we will want to show advanced search terms here, too. While working on this, it occurred to me that the existing TIMDEX UI filter implementation is incomplete. It pulls in the aggregation names and uses those as the internal filter names. This works fine for the existing `contentType` and `source` filters, but it will cause a name collision for filters that also exist as regular search inputs (e.g. `contributors` vs. `contributorsFilter`). Since we will be adding more aggregations and filters soon, it makes sense to fix this problem now. Because this only lists filters at present, we've changed the label from 'You searched for' to 'Applied filters'. This is subject to change. Side effects of these changes: * The "You searched for" list has been removed, as this feature replaces it. As mentioned above, we will likely want to provide that information in some manner in the search summary, but that will require some discussion with Darcy, who is out-of-office at the moment. * Filter keys are now cast as symbols throughout the application. Previously, they would be strings in some places and symbols in others. I found that confusing, but if this change is even more confusing, I'm happy to revert it. * A few tests have been skipped due to the removal of the "You searched for list". I chose not to delete them in case we reintroduce that feature in the near future. * Most of the cassettes have been regenerated, due to changes to the `TimdexSearch` model. Address code review feedback --- app/assets/stylesheets/partials/_panels.scss | 33 +++++ app/assets/stylesheets/partials/_search.scss | 2 +- app/controllers/search_controller.rb | 6 +- app/helpers/filter_helper.rb | 16 ++- app/helpers/form_helper.rb | 5 +- app/models/enhancer.rb | 2 +- app/models/query_builder.rb | 8 +- app/models/timdex_search.rb | 4 +- app/views/search/_filter.html.erb | 2 +- app/views/search/_search_summary.html.erb | 15 ++ app/views/search/results.html.erb | 41 +----- test/controllers/search_controller_test.rb | 14 +- test/helpers/filter_helper_test.rb | 86 ++++++++---- test/helpers/form_helper_test.rb | 16 ++- test/vcr_cassettes/advanced.yml | 26 ++-- test/vcr_cassettes/advanced_all.yml | 22 +-- test/vcr_cassettes/advanced_all_spaces.yml | 20 ++- test/vcr_cassettes/advanced_citation_asdf.yml | 20 ++- test/vcr_cassettes/advanced_keyword_asdf.yml | 29 ++-- .../advanced_source_defaults_to_all.yml | 26 ++-- .../advanced_source_limit_to_one_source.yml | 26 ++-- .../advanced_source_limit_to_two_sources.yml | 26 ++-- test/vcr_cassettes/advanced_title_data.yml | 132 +++--------------- .../alma_record_with_no_publication_date.yml | 49 +++++-- test/vcr_cassettes/data.yml | 26 ++-- test/vcr_cassettes/data_basic_controller.yml | 26 ++-- .../data_from_ridiculous_start.yml | 20 ++- test/vcr_cassettes/data_page_2.yml | 26 ++-- .../timdex_10_1038_nphys1170_.yml | 26 ++-- test/vcr_cassettes/timdex_1234-5678.yml | 42 +++--- test/vcr_cassettes/timdex_9781857988536.yml | 20 ++- test/vcr_cassettes/timdex_PMID_35649707.yml | 26 ++-- .../timdex_controller_record_no_record.yml | 47 +++++-- .../timdex_controller_record_sample.yml | 16 ++- test/vcr_cassettes/timdex_empty_search.yml | 26 ++-- test/vcr_cassettes/timdex_error.yml | 26 ++-- test/vcr_cassettes/timdex_hallo.yml | 29 ++-- test/vcr_cassettes/timdex_no_results.yml | 20 ++- test/vcr_cassettes/timdex_null_search.yml | 26 ++-- .../vcr_cassettes/timdex_record_no_record.yml | 47 +++++-- .../timdex_record_null_record.yml | 16 ++- test/vcr_cassettes/timdex_record_sample.yml | 16 ++- 42 files changed, 641 insertions(+), 466 deletions(-) create mode 100644 app/views/search/_search_summary.html.erb diff --git a/app/assets/stylesheets/partials/_panels.scss b/app/assets/stylesheets/partials/_panels.scss index e40cb284..03ae2e7b 100644 --- a/app/assets/stylesheets/partials/_panels.scss +++ b/app/assets/stylesheets/partials/_panels.scss @@ -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; + } +} diff --git a/app/assets/stylesheets/partials/_search.scss b/app/assets/stylesheets/partials/_search.scss index f8768cd5..cfba3f46 100644 --- a/app/assets/stylesheets/partials/_search.scss +++ b/app/assets/stylesheets/partials/_search.scss @@ -5,7 +5,7 @@ /* basic search bar */ .basic-search { background-color: #989898; - margin-bottom: 1rem; + margin-bottom: 0rem; padding: 1.6rem 2rem; details { diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 82d5bd67..636eb2f9 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -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 } end def extract_results(response) diff --git a/app/helpers/filter_helper.rb b/app/helpers/filter_helper.rb index fe17874d..8a558d3e 100644 --- a/app/helpers/filter_helper.rb +++ b/app/helpers/filter_helper.rb @@ -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 @@ -21,8 +21,8 @@ def add_filter(query, filter, term) def nice_labels { - 'contentType' => 'Content types', - 'source' => 'Sources' + contentTypeFilter: 'Content type', + sourceFilter: 'Source' } end @@ -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 diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 0c65bf12..61b76822 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -2,9 +2,8 @@ module FormHelper def source_checkbox(source, params) "
".html_safe diff --git a/app/models/enhancer.rb b/app/models/enhancer.rb index a9907c27..8945af7f 100644 --- a/app/models/enhancer.rb +++ b/app/models/enhancer.rb @@ -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 = {} diff --git a/app/models/query_builder.rb b/app/models/query_builder.rb index 5b7cbcdc..afb69c88 100644 --- a/app/models/query_builder.rb +++ b/app/models/query_builder.rb @@ -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 = {} @@ -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 diff --git a/app/models/timdex_search.rb b/app/models/timdex_search.rb index e310c09d..45e18d66 100644 --- a/app/models/timdex_search.rb +++ b/app/models/timdex_search.rb @@ -15,7 +15,7 @@ class TimdexSearch < TimdexBase $sourceFilter: [String!] $index: String $from: String - $contentType: [String!] + $contentTypeFilter: [String!] ) { search( searchterm: $q @@ -29,7 +29,7 @@ class TimdexSearch < TimdexBase sourceFilter: $sourceFilter index: $index from: $from - contentTypeFilter: $contentType + contentTypeFilter: $contentTypeFilter ) { hits records { diff --git a/app/views/search/_filter.html.erb b/app/views/search/_filter.html.erb index 0cc9f833..0cb5a6dc 100644 --- a/app/views/search/_filter.html.erb +++ b/app/views/search/_filter.html.erb @@ -1,4 +1,4 @@ -<% return if values.empty? %> +<% return if values.blank? %>