From a32c814de8de6312eff68b1d1d04a521a24395f9 Mon Sep 17 00:00:00 2001 From: Rob Kaufman Date: Tue, 19 Dec 2023 23:19:24 -0800 Subject: [PATCH 1/3] clean up from pr review --- .env | 3 +- app/controllers/catalog_controller.rb | 21 +------ app/search_builders/adv_search_builder.rb | 26 +++++++++ .../adv_search_builder_spec.rb | 57 +++++++++++++++++++ 4 files changed, 85 insertions(+), 22 deletions(-) create mode 100644 app/search_builders/adv_search_builder.rb create mode 100644 spec/search_builders/adv_search_builder_spec.rb diff --git a/.env b/.env index 540f0aacf..8c4081a82 100644 --- a/.env +++ b/.env @@ -14,12 +14,11 @@ FCREPO_BASE_PATH=/hykudemo FCREPO_HOST=fcrepo FCREPO_PORT=8080 FCREPO_REST_PATH=rest -HYRAX_ACTIVE_JOB_QUEUE=good_job +HYRAX_ACTIVE_JOB_QUEUE=sidekiq HYRAX_FITS_PATH=/app/fits/fits.sh INITIAL_ADMIN_EMAIL=admin@example.com INITIAL_ADMIN_PASSWORD=testing123 IN_DOCKER=true -JAVA_OPTS= JAVA_OPTS=-Xmx4g -Xms1g LD_LIBRARY_PATH=/opt/fits/tools/mediainfo/linux NEGATIVE_CAPTCHA_SECRET=default-value-change-me diff --git a/app/controllers/catalog_controller.rb b/app/controllers/catalog_controller.rb index a9105df5b..2df4df825 100644 --- a/app/controllers/catalog_controller.rb +++ b/app/controllers/catalog_controller.rb @@ -64,7 +64,7 @@ def self.uploaded_field config.search_builder_class = IiifPrint::CatalogSearchBuilder # Use locally customized AdvSearchBuilder so we can enable blacklight_advanced_search - # TODO ROB config.search_builder_class = AdvSearchBuilder + config.search_builder_class = AdvSearchBuilder # Show gallery view config.view.gallery.partials = %i[index_header index] @@ -122,22 +122,6 @@ def self.uploaded_field # handler defaults, or have no facets. config.add_facet_fields_to_solr_request! - # TODO: ROB - # # Prior to this change, the applications specific translations were not loaded. Dogbiscuits were assuming the translations were already loaded. - # Rails.root.glob("config/locales/*.yml").each do |path| - # I18n.load_path << path.to_s - # end - # I18n.backend.reload! - # index_props = DogBiscuits.config.index_properties.collect do |prop| - # { prop => index_options(prop, DogBiscuits.config.property_mappings[prop]) } - # end - # add_index_field config, index_props - - # solr fields to be displayed in the show (single result) view - # The ordering of the field names is the order of the display - # show_props = DogBiscuits.config.all_properties - # add_show_field config, show_props - # solr fields to be displayed in the index (search results) view # The ordering of the field names is the order of the display config.add_index_field 'title_tesim', label: "Title", itemprop: 'name', if: false @@ -202,8 +186,6 @@ def self.uploaded_field # since we aren't specifying it otherwise. config.add_search_field('all_fields', label: 'All Fields', include_in_advanced_search: false) do |field| all_names = config.show_fields.values.map(&:field).join(" ") - # TODO: ROB all_names = (config.show_fields.values.map { |v| v.field.to_s } + - # DogBiscuits.config.all_properties.map { |p| "#{p}_tesim" }).uniq.join(" ") title_name = 'title_tesim' field.solr_parameters = { qf: "#{all_names} file_format_tesim all_text_timv", @@ -232,7 +214,6 @@ def self.uploaded_field end config.add_search_field('creator') do |field| - # TODO: ROB field.label = "Author" field.solr_parameters = { "spellcheck.dictionary": "creator" } solr_name = 'creator_tesim' field.solr_local_parameters = { diff --git a/app/search_builders/adv_search_builder.rb b/app/search_builders/adv_search_builder.rb new file mode 100644 index 000000000..b0db273f6 --- /dev/null +++ b/app/search_builders/adv_search_builder.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class AdvSearchBuilder < IiifPrint::CatalogSearchBuilder + include Blacklight::Solr::SearchBuilderBehavior + include BlacklightAdvancedSearch::AdvancedSearchBuilder + + # A Solr param filter that is NOT included by default in the chain, + # but is appended by AdvancedController#index, to do a search + # for facets _ignoring_ the current query, we want the facets + # as if the current query weren't there. + # + # Also adds any solr params set in blacklight_config.advanced_search[:form_solr_parameters] + def facets_for_advanced_search_form(solr_p) + # ensure empty query is all records, to fetch available facets on entire corpus + solr_p["q"] = '{!lucene}*:*' + # explicitly use lucene defType since we are passing a lucene query above (and appears to be required for solr 7) + solr_p["defType"] = 'lucene' + # We only care about facets, we don't need any rows. + solr_p["rows"] = "0" + + # Anything set in config as a literal + if blacklight_config.advanced_search[:form_solr_parameters] + solr_p.merge!(blacklight_config.advanced_search[:form_solr_parameters]) + end + end +end diff --git a/spec/search_builders/adv_search_builder_spec.rb b/spec/search_builders/adv_search_builder_spec.rb new file mode 100644 index 000000000..f2816997e --- /dev/null +++ b/spec/search_builders/adv_search_builder_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +RSpec.describe AdvSearchBuilder do + let(:scope) do + double(blacklight_config: CatalogController.blacklight_config, + current_ability: ability) + end + let(:user) { create(:user) } + let(:ability) { ::Ability.new(user) } + let(:access) { :read } + let(:builder) { described_class.new(scope).with_access(access) } + + it "can be instantiated" do + expect(builder).to be_instance_of(described_class) + end + + describe ".default_processor_chain" do + subject { described_class.default_processor_chain } + + let(:expected_default_processor_chain) do + # Yes there's a duplicate for add_access_controls_to_solr_params; but that does not appear to + # be causing a problem like the duplication and order of the now removed additional + # :add_advanced_parse_q_to_solr, :add_advanced_search_to_solr filters. Those existed in their + # current position and at the end of the array. + # + # When we had those duplicates, the :add_advanced_parse_q_to_solr obliterated the join logic + # for files. + %i[ + default_solr_parameters + add_query_to_solr + add_facet_fq_to_solr + add_facetting_to_solr + add_solr_fields_to_query + add_paging_to_solr + add_sorting_to_solr + add_group_config_to_solr + add_facet_paging_to_solr + add_range_limit_params + add_advanced_parse_q_to_solr + add_advanced_search_to_solr + add_access_controls_to_solr_params + filter_models + only_active_works + add_access_controls_to_solr_params + show_works_or_works_that_contain_files + show_only_active_records + filter_collection_facet_for_access + exclude_models + highlight_search_params + show_parents_only + include_allinson_flex_fields + ] + end + + it { is_expected.to eq(expected_default_processor_chain) } + end +end From f4065ea1c1bb11956f8c687f2b7a8a55d7b5ed00 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 20 Dec 2023 08:28:11 -0500 Subject: [PATCH 2/3] Appease rubocop --- app/search_builders/adv_search_builder.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/search_builders/adv_search_builder.rb b/app/search_builders/adv_search_builder.rb index b0db273f6..92e486f11 100644 --- a/app/search_builders/adv_search_builder.rb +++ b/app/search_builders/adv_search_builder.rb @@ -19,8 +19,6 @@ def facets_for_advanced_search_form(solr_p) solr_p["rows"] = "0" # Anything set in config as a literal - if blacklight_config.advanced_search[:form_solr_parameters] - solr_p.merge!(blacklight_config.advanced_search[:form_solr_parameters]) - end + solr_p.merge!(blacklight_config.advanced_search[:form_solr_parameters]) if blacklight_config.advanced_search[:form_solr_parameters] end end From 1929072d38a41b4420bd1cb936692c4fc6848e97 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 21 Dec 2023 17:19:25 -0500 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A7=B9=20Rearrange=20order=20of=20fil?= =?UTF-8?q?ter=20chain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is not tested in the UI, but the `show_works_or_works_that_contain_files` remains after the troublesome advanced query filters. --- spec/search_builders/adv_search_builder_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/search_builders/adv_search_builder_spec.rb b/spec/search_builders/adv_search_builder_spec.rb index f2816997e..f7352edec 100644 --- a/spec/search_builders/adv_search_builder_spec.rb +++ b/spec/search_builders/adv_search_builder_spec.rb @@ -25,8 +25,12 @@ # # When we had those duplicates, the :add_advanced_parse_q_to_solr obliterated the join logic # for files. + # + # Is the order immutable? No. But it does highlight that you must consider what the changes + # might mean and double check that join logic on files. %i[ default_solr_parameters + add_search_field_default_parameters add_query_to_solr add_facet_fq_to_solr add_facetting_to_solr @@ -35,12 +39,14 @@ add_sorting_to_solr add_group_config_to_solr add_facet_paging_to_solr + add_adv_search_clauses + add_additional_filters add_range_limit_params - add_advanced_parse_q_to_solr - add_advanced_search_to_solr add_access_controls_to_solr_params filter_models only_active_works + add_advanced_parse_q_to_solr + add_advanced_search_to_solr add_access_controls_to_solr_params show_works_or_works_that_contain_files show_only_active_records