Skip to content
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.

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

[WIP] Enable a configuration for skipping a saved search that uses the requ… #2153

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 16 additions & 2 deletions app/controllers/concerns/blacklight/search_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module Blacklight::SearchContext

class_methods do
# Save the submitted search parameters in the search session
def record_search_parameters opts = { only: :index }
def record_search_parameters(opts = { unless: proc { |c| (c.action_name.to_s != "index") || c.send(:skip_session_tracking?) } })
before_action :set_current_search_session, opts
end
end
Expand All @@ -39,7 +39,7 @@ def set_current_search_session
end

def find_search_session
if agent_is_crawler?
if skip_session_tracking?
nil
elsif params[:search_context].present?
find_or_initialize_search_session_from_params JSON.parse(params[:search_context])
Expand Down Expand Up @@ -79,6 +79,20 @@ def agent_is_crawler?
crawler_proc.call(request)
end

def skip_session_tracking?
Copy link
Member

Choose a reason for hiding this comment

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

I haven't look at what track_search_session is, but what if we let it be a boolean and/or a lambda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that might be a better extendable approach. I'll look into that.

return true if agent_is_crawler?

skip_session_proc = blacklight_config.skip_session_tracking
case skip_session_proc
when TrueClass
true
when Proc
skip_session_proc.call(request, params)
else
false
end
end

def find_or_initialize_search_session_from_params params
params_copy = params.reject { |k, v| blacklisted_search_session_params.include?(k.to_sym) || v.blank? }

Expand Down
5 changes: 5 additions & 0 deletions lib/blacklight/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ def default_values
# @return [Boolean]
property :track_search_session, default: true

# @!attribute skip_session_tracking
# @since v8.0.0
# @return [Boolean]
property :skip_session_tracking, default: nil

# @!attribute advanced_search
# @since v7.15.0
# @return [#enabled]
Expand Down
26 changes: 26 additions & 0 deletions spec/features/skip_session_tracking_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

RSpec.describe 'Search session skipping' do
describe 'crawler search' do
let(:original_proc) { ::CatalogController.blacklight_config.skip_session_tracking }

before do
::CatalogController.blacklight_config.skip_session_tracking = ->(_req, params) { params.fetch('view', nil) == 'weird_json_view' }
end

after do
::CatalogController.blacklight_config.skip_session_tracking = original_proc
end

it 'remembers most searches' do
visit root_path
fill_in 'q', with: 'chicken'
expect { click_button 'search' }.to change(Search, :count).by(1)
end

it 'does not remember weird json search' do
visit root_path
expect { visit search_catalog_path(q: 'chicken', view: 'weird_json_view') }.not_to change(Search, :count)
end
end
end