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

Store and retrieve "search history context" in the database #619

Merged
merged 9 commits into from Oct 18, 2013

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Sep 23, 2013

The current "search history context" feature (that allows you to paginate through a results set from a record view) currently relies on passing a nasty hash of parameters through the session object (which is unsurprisingly brittle, and breaks in a multi-tab environment - see #605)

This PR piggybacks on the existing search history feature (which is storing the query parameters in the database already), and passes along only the search ID, and we'll look up the query parameters from the database, not the session.

Finally, this incorporates the core of #605, except using that ID rather than the full params hash.

@cbeer
Copy link
Member Author

cbeer commented Sep 24, 2013

@elo2112 could you review this (in particular, 06ed54f) and make sure it satisfies the use cases from #605? I think I was able to simplify your implementation by refactoring how the search history works; hopefully this hit all the edge cases.

@elohanlon
Copy link
Contributor

It looks like you're storing the user's last 12 searches (SearchHistoryWindow = 12), giving them ids, and passing an id along instead of all search params. I like that this strategy avoids passing all of the param data, but it looks problematic in this case:

  1. A user goes to your site and performs a search in Tab 1.

  2. The user takes a quick look at the search results in Tab 1, likes them, and keeps that tab open so that he can take a closer look through them later.

  3. The user opens a new tab, Tab 2, and does a slightly different search, but doesn't find the right results. The user performs several more searches in Tab 2 to find what he wants, and either (A) modifies his search terms a couple of times, (B) adds or removes a bunch of facets, or changes the result sort order (and each addition/removal/sort counts as a new search) or (C) decides to explore the site a bit, and goes on to perform several more unrelated searches for that reason. During this search tweaking / exploration, he easily performs more than 12 searches, which are all saved in his search history, bumping out old searches like the search from Tab 1.

  4. The user, finally having found what he was looking for in Tab 2, goes back to Tab 1 and continues to page through results...but this search has already been removed from the search history because it's more than 12 searches old, so he's unable to continue where he left off.

I checked out your search-history-refactor branch to test out this scenario, and when I tried to continue with the search in Tab 1, I lost my previous and next links on the item level view.

So I guess the question is...how big should this SearchHistoryWindow value be? 30? 50? More? The more you have, the more temporary rows you're writing to the database. When I was originally trying to solve the multi-tab search problem, I thought about a method like this, but this scenario is what made me decide to pass all of the search params. It's a bit of extra data for each request, but it allows for an infinite number of searches between tabs while still retaining the search result set in each tab. Even in the case of a moderately complex search (two search terms, four facets, a specified sort field and a specified per-page count), you would be passing less than half a kilobyte of extra text data:

Blacklight.last_known_search_json_string = "{"f":{"format":["photographs"],"state_or_province__facet":["California"],"subject_geographic__facet":["Jackson (Calif.)"],"subject_name__facet":["United States. Bureau of Indian Affairs. Digger Indian Agency"]},"per_page":"60","q":"native american","search":"true","sort":"item_title__sort asc","utf8":"✓","action":"index","controller":"catalog","total":4}";

@cbeer
Copy link
Member Author

cbeer commented Sep 24, 2013

That's a good point.

I think expanding the history window shouldn't be a problem though. As I read it, we're actually always writing the row to the database (and letting it get garbage collected later), it just falls out of the session.

@cbeer
Copy link
Member Author

cbeer commented Sep 24, 2013

I talked with @jkeck about somehow signing the search_id parameters, etc, etc to get around end-user tampering. I'm not sure the added complexity is actually worth it, in the end, and provides no additional security beyond what the session[:history] check is already providing.

@cbeer
Copy link
Member Author

cbeer commented Sep 24, 2013

@elo2112 I've tried to address your concerns above. After talking with @jkeck and @MrDys, I still think passing around the search id probably meets the 80% need (especially after raising, and making configurable, the size of the search history window).

@jkeck had a good use case that suggested a hybrid approach (see 2c0d7d8), where he'd like to link to a document with a explicit search context (e.g. from a record page, clicking on a browse nearby link and having the search context controls somehow reflect that). I suspect that could be reworked fairly easily to address your use case as well, and may be as easy as overriding the search_session_params helper to spit out the query parameters (much like last_known_search_string) instead of the search id.

@elohanlon
Copy link
Contributor

Sounds good. And I like the idea of being able to link to a document with an explicit search context. Very useful. Thanks for all of your work on this!

jcoyne added a commit that referenced this pull request Oct 18, 2013
refactor search history logic so we're pulling search query parameters o...
@jcoyne jcoyne merged commit 17d4665 into master Oct 18, 2013
@jcoyne jcoyne deleted the search-history-refactor branch October 18, 2013 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants