From dd3dc07fcad684c8df103b7eba284de09ebe43c0 Mon Sep 17 00:00:00 2001 From: Amrey Mathurin Date: Wed, 14 Aug 2024 21:19:07 -0400 Subject: [PATCH 1/3] Prevent duplicate hidden fields in adv search form --- .../blacklight/advanced_search_form_component.html.erb | 2 +- app/components/blacklight/advanced_search_form_component.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/components/blacklight/advanced_search_form_component.html.erb b/app/components/blacklight/advanced_search_form_component.html.erb index 3ebef7168..b42bf32fa 100644 --- a/app/components/blacklight/advanced_search_form_component.html.erb +++ b/app/components/blacklight/advanced_search_form_component.html.erb @@ -8,7 +8,7 @@ <% end %> <%= form_tag @url, method: @method, class: @classes.join(' '), role: 'search', 'aria-label' => t('blacklight.search.form.submit') do %> - <%= render Blacklight::HiddenSearchStateComponent.new(params: @params) %> + <%= render Blacklight::HiddenSearchStateComponent.new(params: hidden_search_state_params) %>
diff --git a/app/components/blacklight/advanced_search_form_component.rb b/app/components/blacklight/advanced_search_form_component.rb index de632fdcc..0cfa55c31 100644 --- a/app/components/blacklight/advanced_search_form_component.rb +++ b/app/components/blacklight/advanced_search_form_component.rb @@ -34,6 +34,12 @@ def sort_fields_select select_tag(:sort, options_for_select(options, params[:sort]), class: "form-select custom-select sort-select w-auto", aria: { labelledby: 'advanced-search-sort-label' }) end + # Filtered params to pass to hidden search fields + # @return [ActiveSupport::HashWithIndifferentAccess] + def hidden_search_state_params + @params.except(:clause, :f_inclusive, :op, :sort) + end + private def initialize_search_field_controls From c4e3be6ea413d9748032ddc104673abc1c1a1cee Mon Sep 17 00:00:00 2001 From: Amrey Mathurin Date: Wed, 14 Aug 2024 21:20:22 -0400 Subject: [PATCH 2/3] Apply incoming inclusive facets to adv search form --- .../facet_field_checkboxes_component.rb | 2 +- .../facet_checkbox_item_presenter.rb | 11 +++++ spec/features/advanced_search_spec.rb | 18 ++++++- .../facet_checkbox_item_presenter_spec.rb | 48 +++++++++++++++++++ 4 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 app/presenters/blacklight/facet_checkbox_item_presenter.rb create mode 100644 spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb diff --git a/app/components/blacklight/facet_field_checkboxes_component.rb b/app/components/blacklight/facet_field_checkboxes_component.rb index c98eac844..dd1ff59ac 100644 --- a/app/components/blacklight/facet_field_checkboxes_component.rb +++ b/app/components/blacklight/facet_field_checkboxes_component.rb @@ -17,7 +17,7 @@ def presenters return to_enum(:presenters) unless block_given? @facet_field.paginator.items.each do |item| - yield @facet_field.facet_field.item_presenter.new(item, @facet_field.facet_field, helpers, @facet_field.key, @facet_field.search_state) + yield Blacklight::FacetCheckboxItemPresenter.new(item, @facet_field.facet_field, helpers, @facet_field.key, @facet_field.search_state) end end end diff --git a/app/presenters/blacklight/facet_checkbox_item_presenter.rb b/app/presenters/blacklight/facet_checkbox_item_presenter.rb new file mode 100644 index 000000000..0284ea53a --- /dev/null +++ b/app/presenters/blacklight/facet_checkbox_item_presenter.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Blacklight + class FacetCheckboxItemPresenter < Blacklight::FacetItemPresenter + # Check if the query parameters have any inclusive facets with the given value + # @return [Boolean] + def selected? + search_state.filter(facet_config).values(except: [:filters, :missing]).flatten.include?(value) + end + end +end diff --git a/spec/features/advanced_search_spec.rb b/spec/features/advanced_search_spec.rb index 89d934f37..fdf0aae47 100644 --- a/spec/features/advanced_search_spec.rb +++ b/spec/features/advanced_search_spec.rb @@ -105,11 +105,23 @@ describe "prepopulated advanced search form" do before do - visit '/catalog/advanced?op=must&clause[0][field]=title&clause[0]query=medicine' + visit '/catalog/advanced?op=must&clause[1][field]=title&clause[1]query=medicine&f_inclusive[language_ssim][]=Tibetan&sort=author' end - it "does not create hidden inputs for search fields" do + it 'prepopulates the expected fields' do expect(page).to have_field 'Title', with: 'medicine' + expect(page).to have_field 'Tibetan', checked: true + expect(page).to have_select 'op', selected: 'all' + expect(page).to have_select 'sort', selected: 'author' + end + + it "does not create hidden inputs for fields included in adv search form" do + within('form.advanced') do + expect(page).to have_no_field('clause[1][query]', type: :hidden, with: 'medicine') + expect(page).to have_no_field('f_inclusive[language_ssim][]', type: :hidden, with: 'Tibetan') + expect(page).to have_no_field('op', type: :hidden, with: 'must') + expect(page).to have_no_field('sort', type: :hidden, with: 'author') + end end it "does not have multiple parameters for a search field" do @@ -121,8 +133,10 @@ it "clears the prepopulated fields when the Start Over button is pressed" do expect(page).to have_field 'Title', with: 'medicine' + expect(page).to have_field 'Tibetan', checked: true click_on 'Start over' expect(page).to have_no_field 'Title', with: 'medicine' + expect(page).to have_no_field 'Tibetan', checked: true end end end diff --git a/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb b/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb new file mode 100644 index 000000000..b541776a3 --- /dev/null +++ b/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Blacklight::FacetCheckboxItemPresenter, type: :presenter do + subject(:presenter) do + described_class.new(facet_item, facet_config, view_context, facet_field, search_state) + end + + let(:facet_item) { Blacklight::Solr::Response::Facets::FacetItem.new(value: 'Book', hits: 30) } + let(:facet_config) { Blacklight::Configuration::FacetField.new(key: 'format') } + let(:view_context) { controller.view_context } + let(:filter_field) { Blacklight::SearchState::FilterField.new(facet_config, search_state) } + let(:facet_field) { Blacklight::Solr::Response::Facets::FacetField.new('format', [facet_item]) } + let(:params) { ActionController::Parameters.new(f_inclusive: { format: ["Book"] }) } + let(:blacklight_config) { Blacklight::Configuration.new } + let(:search_state) { Blacklight::SearchState.new(params, blacklight_config) } + + before do + blacklight_config.add_facet_field 'format' + end + + describe '#selected?' do + subject { presenter.selected? } + + context 'with a matching inclusive filter' do + it 'returns true' do + expect(subject).to be true + end + end + + context 'with an inclusive filter that does not match' do + let(:params) { ActionController::Parameters.new(f_inclusive: { format: ["Manuscript"] }) } + + it 'returns true' do + expect(subject).to be false + end + end + + context 'with a matching exclusive filter' do + let(:params) { ActionController::Parameters.new(f: { format: ["Book"] }) } + + it 'returns false' do + expect(subject).to be false + end + end + end +end From 2760817380bab917e631191e90e08c408efc900c Mon Sep 17 00:00:00 2001 From: Amrey Mathurin Date: Wed, 21 Aug 2024 11:17:20 -0400 Subject: [PATCH 3/3] Correct misleading test description - use rspec `is_expected` syntax to simplify tests --- .../blacklight/facet_checkbox_item_presenter_spec.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb b/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb index b541776a3..9bccba5f2 100644 --- a/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb +++ b/spec/presenters/blacklight/facet_checkbox_item_presenter_spec.rb @@ -24,25 +24,19 @@ subject { presenter.selected? } context 'with a matching inclusive filter' do - it 'returns true' do - expect(subject).to be true - end + it { is_expected.to be true } end context 'with an inclusive filter that does not match' do let(:params) { ActionController::Parameters.new(f_inclusive: { format: ["Manuscript"] }) } - it 'returns true' do - expect(subject).to be false - end + it { is_expected.to be false } end context 'with a matching exclusive filter' do let(:params) { ActionController::Parameters.new(f: { format: ["Book"] }) } - it 'returns false' do - expect(subject).to be false - end + it { is_expected.to be false } end end end