[#4552] More reliable boolean operators on the advanced search form (#4553)

* [#4552] More reliable boolean operators on the advanced search form

This is not quite working, see failing test
spec/features/advanced_searching_spec.rb:156

Specifically, if the user enters a boolean NOT search in the advanced
search, the constraints on the search results page is currently not
including the word NOT, so the user doesn't get the feedback that it
is a negated term.

I think we could address this in our ClausePresenter, although it
might make that class quite a bit more complex.

* Mutate the params in the controller instead

This allows the ClausePresenter to access both our params (e.g. boolean_operator1=OR)
and the ones that Solr's BoolQParser likes (e.g. clause[0][op]=should).

Author: sandbergja

.reek.yml

@@ -50,6 +50,7 @@ detectors:
     - Requests::ApplicationHelper#hidden_service_options
     - Requests::ApplicationHelper#single_pickup
     - Requests::RequestHelper#reshare_type
+    - AdvancedBooleanOperators#initialize
     - Blacklight::Document::JsonLd#check_role
     - Blacklight::Document::JsonLd#date
     - Requests::Aeon#holding_location_to_site
@@ -62,16 +63,12 @@ detectors:
     - PhysicalHoldingsMarkupBuilder#self.thesis?
   DataClump:
     exclude:
-    - ApplicationHelper
     - Requests::ApplicationHelper
     - Requests::SelectedItemsValidator
     - PhysicalHoldingsMarkupBuilder
   DuplicateMethodCall:
     exclude:
     - Holdings::OnlineHoldingsComponent#marc_links
-    - LocationCodeFacetComponent#add_scsb_loc
-    - LocationCodeFacetComponent#fetch_libraries_and_locations
-    - LocationCodeFacetComponent#library_facet_values
     - NumismaticsSearchFormComponent#initialize_constraints
     - Orangelight::AdvancedSearchFormComponent#fields_for_etc
     - Orangelight::AdvancedSearchFormComponent#initialize_search_filter_controls
@@ -86,7 +83,6 @@ detectors:
     - ApplicationController#default_url_options
     - ApplicationController#origin
     - BookmarksController#csv_values
-    - BookmarksController#index
     - CatalogController#render_empty_search
     - Orangelight::Catalog#linked_records
     - Orangelight::Catalog#redirect_browse
@@ -151,7 +147,6 @@ detectors:
     - Requests::RequestHelper#pul_patron_name
     - Requests::RequestMailer#on_shelf_email
     - Requests::RequestMailer#paging_pick_ups
-    - Blacklight::Document::Email#add_holding_fields
     - Blacklight::Document::Email#add_online_text
     - Blacklight::Document::JsonLd#data
     - Blacklight::Document::JsonLd#date
@@ -171,7 +166,7 @@ detectors:
     - Requests::Scsb#parse_scsb_response
     - Requests::Scsb#scsb_param_mapping
     - Requests::Scsb#scsb_request
-    - RecordMailer#email_record
+    - Orangelight::RecordMailer#email_record
     - Requests::AeonUrl#ctx_with_item_info
     - Requests::ClancyItem#get_clancy
     - Requests::ClancyItem#initialize
@@ -252,7 +247,6 @@ detectors:
     - StackmapService::Url#url
   FeatureEnvy:
     exclude:
-    - LocationCodeFacetComponent#add_scsb_loc
     - Orangelight::ConstraintsComponent#guided_search_value
     - Orangelight::FacetFieldCheckboxesComponent#values
     - Orangelight::SearchBarComponent#search_fields
@@ -262,7 +256,6 @@ detectors:
     - AdvancedHelper#advanced_search_fields
     - AdvancedHelper#param_for_field
     - ApplicationHelper#action_notes_display
-    - ApplicationHelper#search_location_display
     - ApplicationHelper#series_results
     - ApplicationHelper#title_hierarchy
     - BlacklightHelper#isbn_resolve
@@ -292,7 +285,7 @@ detectors:
     - Requests::Scsb#parse_scsb_response
     - Requests::Scsb#scsb_param_mapping
     - Requests::Scsb#scsb_request
-    - RecordMailer#email_record
+    - Orangelight::RecordMailer#email_record
     - Requests::AlmaPatron#active_barcode
     - Requests::ClancyItem#get_clancy
     - Requests::ClancyItem#post_clancy
@@ -384,7 +377,6 @@ detectors:
     - IndexMetadataComponent
     - IndexMetadataFieldLayoutComponent
     - IndexTitleComponent
-    - LocationCodeFacetComponent
     - MultiselectComboboxComponent
     - NumismaticsSearchFormComponent
     - Orangelight::AdvancedSearchFormComponent
@@ -395,6 +387,7 @@ detectors:
     - Orangelight::FacetFieldCheckboxesComponent
     - Orangelight::SearchBarComponent
     - Orangelight::SearchContext::ServerItemPaginationComponent
+    - Orangelight::System::FlashMessageComponent
     - AccountController
     - ApplicationController
     - BookmarksController
@@ -442,6 +435,7 @@ detectors:
     - Orangelight::CallNumber
     - Orangelight::Name
     - Orangelight::NameTitle
+    - Orangelight::RecordMailer
     - Orangelight::Subject
     - Orangelight
     - Requests::ClancyItem
@@ -511,8 +505,6 @@ detectors:
   LongParameterList:
     exclude:
     - Orangelight::ConstraintsComponent#initialize
-    - ApplicationHelper#locate_link_with_glyph
-    - ApplicationHelper#locate_url
     - HoldingsHelper#holding_status_li
     - Requests::ApplicationHelper#single_pickup
     - Requests::RequestMailer#confirmation_email
@@ -546,7 +538,6 @@ detectors:
     - ApplicationController
   NestedIterators:
     exclude:
-    - LocationCodeFacetComponent#fetch_libraries_and_locations
     - BookmarksController#csv_output
     - AdvancedHelper#generate_solr_fq
     - ApplicationHelper#name_title_hierarchy
@@ -568,10 +559,6 @@ detectors:
     - SolrDocument#identifiers
   NilCheck:
     exclude:
-    - LocationCodeFacetComponent#add_library
-    - LocationCodeFacetComponent#add_scsb_loc
-    - LocationCodeFacetComponent#fetch_libraries_and_locations
-    - LocationCodeFacetComponent#location_codes_by_lib
     - Orangelight::AdvancedSearchFormComponent#initialize_search_filter_controls
     - AccountController#cancel_ill_requests
     - ApplicationController#after_sign_in_path_for
@@ -589,7 +576,6 @@ detectors:
     - ApplicationHelper#alma_location_display
     - ApplicationHelper#fast_subjects_value?
     - ApplicationHelper#holding_location_label
-    - ApplicationHelper#locate_link_with_glyph
     - ApplicationHelper#render_location_code
     - BlacklightHelper#html_facets
     - BlacklightHelper#isbn_resolve
@@ -647,7 +633,7 @@ detectors:
   TooManyInstanceVariables:
     exclude:
     - MultiselectComboboxComponent
-    - BookmarksController
+    - CatalogController
     - FeedbackController
     - Orangelight::BrowsablesController
     - Requests::FormController
@@ -689,13 +675,10 @@ detectors:
     - AeonStatus#check!
     - BibdataStatus#check!
     - Holdings::OnlineHoldingsComponent#marc_links
-    - LocationCodeFacetComponent#fetch_libraries_and_locations
-    - LocationCodeFacetComponent#location_codes_by_lib
     - Orangelight::ConstraintsComponent#guided_search_value
     - Orangelight::SearchBarComponent#before_render
     - AccountController#cancel_ill_requests
     - ApplicationController#after_sign_in_path_for
-    - BookmarksController#index
     - CatalogController#numismatics
     - CatalogController#render_empty_search
     - Orangelight::Stackmap#stackmap
@@ -741,7 +724,6 @@ detectors:
     - Blacklight::Document::CiteProc#properties
     - Blacklight::Document::DublinCore#export_as_oai_dc_xml
     - Blacklight::Document::DublinCore#export_as_rdf_dc
-    - Blacklight::Document::Email#add_holdings_text
     - Blacklight::Document::Email#add_online_text
     - Blacklight::Document::JsonLd#contributors
     - Blacklight::Document::JsonLd#data
@@ -760,7 +742,7 @@ detectors:
     - Requests::Scsb#items_by_id
     - Requests::Scsb#parse_scsb_response
     - Requests::Scsb#scsb_request
-    - RecordMailer#email_record
+    - Orangelight::RecordMailer#email_record
     - Requests::AeonUrl#ctx_with_item_info
     - Requests::ClancyItem#post_clancy
     - Requests::ClancyItem#request
@@ -824,8 +806,6 @@ detectors:
     exclude:
     - Orangelight::AdvancedSearchFormComponent#initialize_search_filter_controls
     - Orangelight::ConstraintsComponent#remove_guided_query_path
-    - BookmarkButtonComponent
-    - BookmarksController#bookmark_ids
     - BookmarksController#csv_bom
     - BookmarksController#two_values
     - Orangelight::Catalog#online_holding_note?
@@ -871,13 +851,10 @@ detectors:
     - Requests::RequestMailer#recap_marquand_in_library_email
   UtilityFunction:
     exclude:
-    - LocationCodeFacetComponent#add_library
-    - LocationCodeFacetComponent#library_facet_values
     - Orangelight::DropdownHelpTextComponent#option_text_and_value
     - AccountController#cancel_ill_success
     - AccountController#current_patron
     - ApplicationController#default_url_options
-    - BookmarksController#convert_to_alma_id
     - BookmarksController#csv_bom
     - BookmarksController#two_values
     - Orangelight::Catalog#online_holding_note?
@@ -896,7 +873,6 @@ detectors:
     - ApplicationHelper#holding_location
     - ApplicationHelper#holding_request_block
     - ApplicationHelper#html_safe
-    - ApplicationHelper#locate_url
     - ApplicationHelper#location_full_display
     - ApplicationHelper#rails_env?
     - ApplicationHelper#remote_storage?
@@ -932,7 +908,6 @@ detectors:
     - Requests::RequestMailer#annex_items
     - Bookmark#alma_id?
     - Bookmark#special_system_id?
-    - Blacklight::Document::Email#add_holding_fields
     - Blacklight::Document::Email#add_online_text
     - Blacklight::Document::Email#add_single_valued_field
     - Blacklight::Document::JsonLd#check_role

app/controllers/catalog_controller.rb

@@ -693,7 +693,7 @@ class CatalogController < ApplicationController
 
     config.filter_search_state_fields = true
     config.search_state_fields = config.search_state_fields + [
-      :advanced_type, :clause
+      :advanced_type, :clause, :boolean_operator1, :boolean_operator2
     ]
 
     config.index.constraints_component = Orangelight::ConstraintsComponent
@@ -709,6 +709,7 @@ def render_search_results_as_json
   end
 
   def index
+    solrize_boolean_params
     if no_search_yet?
       render_empty_search
     else
@@ -859,4 +860,8 @@ def search_algorithm_param
     def search_service_compatibility_wrapper
       @search_service_compatibility_wrapper ||= SearchServiceCompatibilityWrapper.new(search_service)
     end
+
+    def solrize_boolean_params
+      @search_state = search_state.reset(AdvancedBooleanOperators.new(search_state.params).for_boolqparser)
+    end
 end

app/models/advanced_boolean_operators.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+# This class is responsible for reading some boolean operators
+# (AND, OR, or NOT) from some URL query params, and translating
+# them into the operators that Solr's BoolQParser likes (must,
+# must_not, or should)
+class AdvancedBooleanOperators
+  def initialize(parameters)
+    @parameters = parameters || ActionController::Parameters.new
+  end
+
+  def for_boolqparser
+    parameters.without(:boolean_operator1, :boolean_operator2).deep_merge(clauses)
+  end
+
+    private
+
+      attr_reader :parameters
+
+      def first_operator
+        parameters['boolean_operator1']&.upcase
+      end
+
+      def second_operator
+        parameters['boolean_operator2']&.upcase
+      end
+
+      def clauses
+        if first_operator.present? || second_operator.present?
+          { clause: { '0': { op: first_boolqparser_param }, '1': { op: middle_boolqparser_param }, '2': { op: last_boolqparser_param } } }
+        else
+          {}
+        end
+      end
+
+      def first_boolqparser_param
+        return 'must' if first_operator == 'AND'
+        return 'must' if first_operator == 'NOT' && second_operator != 'OR'
+        'should'
+      end
+
+      def middle_boolqparser_param
+        return 'should' if second_operator == 'OR' && first_operator != 'NOT'
+        mapping = {
+          'OR' => 'should',
+          'NOT' => 'must_not',
+          'AND' => 'must'
+        }
+        mapping[first_operator] || 'should'
+      end
+
+      def last_boolqparser_param
+        mapping = {
+          'OR' => 'should',
+          'NOT' => 'must_not',
+          'AND' => 'must'
+        }
+        mapping[second_operator] || 'should'
+      end
+end

app/views/advanced/_guided_search_fields.html.erb

@@ -21,9 +21,9 @@
     </div>
   </div>
   <div class="search-operators" role="radiogroup" aria-label="Search operators">
-    <label><%= radio_button_tag("clause[1][op]", "must", guided_radio(:clause_1_op, "AND"), class: 'first-and') %> AND</label>
-    <label><%= radio_button_tag("clause[1][op]", "should", guided_radio(:clause_1_op, "OR"), class: 'first-or') %> OR</label>
-    <label><%= radio_button_tag("clause[1][op]", "must_not", guided_radio(:clause_1_op, "NOT"), class: 'first-not') %> NOT</label>
+    <label><%= radio_button_tag("boolean_operator1", "AND", guided_radio(:boolean_operator1, "AND"), class: 'first-and') %> AND</label>
+    <label><%= radio_button_tag("boolean_operator1", "OR", guided_radio(:boolean_operator1, "OR"), class: 'first-or') %> OR</label>
+    <label><%= radio_button_tag("boolean_operator1", "NOT", guided_radio(:boolean_operator1, "NOT"), class: 'first-not') %> NOT</label>
   </div>
   <div class="search-field">
     <label for="clause_1_field" class="sr-only">Options for advanced search - second parameter</label>
@@ -47,9 +47,9 @@
     </div>
   </div>
   <div class="search-operators" role="radiogroup" aria-label="Search operators">
-    <label><%= radio_button_tag("clause[2][op]", "must", guided_radio(:clause_2_op, "AND")) %> AND</label>
-    <label><%= radio_button_tag("clause[2][op]", "should", guided_radio(:clause_2_op, "OR")) %> OR</label>
-    <label><%= radio_button_tag("clause[2][op]", "must_not", guided_radio(:clause_2_op, "NOT")) %> NOT</label>
+    <label><%= radio_button_tag("boolean_operator2", "AND", guided_radio(:boolean_operator2, "AND")) %> AND</label>
+    <label><%= radio_button_tag("boolean_operator2", "OR", guided_radio(:boolean_operator2, "OR")) %> OR</label>
+    <label><%= radio_button_tag("boolean_operator2", "NOT", guided_radio(:boolean_operator2, "NOT")) %> NOT</label>
   </div>
   <div class="search-field">
     <label for="clause_2_field" class="sr-only">Options for advanced search - third parameter</label>

spec/features/advanced_searching_spec.rb

@@ -123,7 +123,7 @@
     it 'can exclude terms from the search', js: false do
       # defaults to keyword
       fill_in(id: 'clause_0_query', with: 'gay')
-      choose(id: 'clause_2_op_must_not')
+      choose(id: 'boolean_operator2_NOT')
       # defaults to title
       fill_in(id: 'clause_2_query', with: 'RenoOut')
       click_button('advanced-search-submit')
@@ -132,10 +132,24 @@
       expect(page).not_to have_content('Reno Gay Press and Promotions')
     end
 
+    it 'can do a boolean OR search', js: false do
+      # defaults to keyword
+      fill_in(id: 'clause_0_query', with: 'gay')
+      choose(id: 'boolean_operator1_OR')
+      # defaults to title
+      select('Title', from: 'clause_1_field')
+      fill_in(id: 'clause_1_query', with: 'algebra')
+      click_button('advanced-search-submit')
+      expect(page.find(".page_entries").text).to eq('1 - 3 of 3')
+      expect(page).to have_content('Seeking sanctuary')
+      expect(page).to have_content('Reno Gay Press and Promotions')
+      expect(page).to have_content('College algebra')
+    end
+
     it 'shows constraint-value on search results page' do
       # defaults to keyword
       fill_in(id: 'clause_0_query', with: 'gay')
-      choose(id: 'clause_2_op_must_not')
+      choose(id: 'boolean_operator2_NOT')
       # defaults to title
       fill_in(id: 'clause_2_query', with: 'RenoOut')
       click_button('advanced-search-submit')