-
Notifications
You must be signed in to change notification settings - Fork 257
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
base: main
Are you sure you want to change the base?
Conversation
@@ -38,7 +38,7 @@ def set_current_search_session | |||
end | |||
|
|||
def find_search_session | |||
if agent_is_crawler? | |||
if agent_is_crawler? || skip_session_tracking? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go ahead and push agent_is_crawler?
into the skip_session_tracking?
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, should this check get pushed up to record_search_parameters
or before_action :set_current_search_session, if: ...
around line 15?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -78,6 +78,13 @@ def agent_is_crawler? | |||
crawler_proc.call(request) | |||
end | |||
|
|||
def skip_session_tracking? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…est and params object
8f8c025
to
4e0cc07
Compare
Not sure if people still want this, but it was in an awkward state of failing tests/requested changes/approved/needed rebase. I've rebased and put the fixups originally requested in. Do we still want this, or can we close this PR? |
4e0cc07
to
2ef9883
Compare
2ef9883
to
7b812ba
Compare
…est and params object
Very similar to approach for crawler, but semantically a bit different?
What do others think?