Daccess 412 advanced search empty rows fix #2218
Conversation
…to override blacklight
…SS-412-advanced_search_empty_rows_fix
| @@ -1,4 +1,7 @@ | |||
| class ApplicationController < ActionController::Base | |||
| # include ConsoleColors if CONSOLE_COLORS_ENABLED=true in dev.env | |||
| include ConsoleColors if ENV["CONSOLE_COLORS_ENABLED"] == "true" | |||
|
|
|||
There was a problem hiding this comment.
It might be nice to split out this ConsoleColors work into a separate PR so other folks can take a look? What do you think?
There was a problem hiding this comment.
Yeah I could do that. 👍
| # For advanced search, only start a new session if query is present | ||
| if params[:search_field] == "advanced" | ||
| query, index = params[:q_row], action_name == "index" | ||
| start_new_search_history_record = index && !(query[0].blank?) |
There was a problem hiding this comment.
Should this check if any query in the array is not blank, not just the first? The first row could be blank, but the other rows could have values, and we'd still want to track this search in history.
| query_params["search_field_row"] = filtered_indices.map { |i| search_field_row[i] } if search_field_row.present? | ||
| query_params["q_row"] = filtered_indices.map { |i| advanced_query[i] } | ||
| query_params["boolean_row"] = boolean_row.select { |key, _| filtered_indices.include?(key.to_i) } if boolean_row.present? | ||
| query_params["op_row"] = filtered_indices.map { |i| operator_row[i] } |
There was a problem hiding this comment.
This will actually change the search params stored in the search history, not just for display, which leads to the following behavior for this example advanced search with an empty 3rd query row:
- Go to: http://localhost:9292/edit?boolean_row%5B1%5D=AND&boolean_row%5B2%5D=OR&boolean_row%5B3%5D=NOT&op_row%5B%5D=AND&op_row%5B%5D=AND&op_row%5B%5D=begins_with&op_row%5B%5D=OR&q_row%5B%5D=testing&q_row%5B%5D=stuff&q_row%5B%5D=&q_row%5B%5D=things&search_field_row%5B%5D=publisher&search_field_row%5B%5D=subject&search_field_row%5B%5D=journaltitle&search_field_row%5B%5D=title
- Click Search
- Note that Solr query is:
(((publisher:"testing") AND (subject:"stuff")) NOT (title:"things")) - Go go "Search history"
- See search history displayed as:
Publisher: testingAND Subject: stuffANY Title: things- which loses the final NOT boolean. - Click on that search history
- Note the different solr query generated:
(((publisher:"testing") AND (subject:"stuff")) AND (title:"things")). If you go back to the search history page, you'll see a 2nd search history row created for this new search.
Would it be better to handle this param cleanup in the #render_search_to_s_element method, instead of changing the search object?
| # *** Only Overrides Blacklight Helper Methods You Add *** | ||
| # ------------------------------------------------------------------------------ | ||
| Rails.application.config.to_prepare do | ||
| Blacklight::SearchHistoryConstraintsHelperBehavior.prepend(CornellSearchHistoryConstraintsHelper) |
There was a problem hiding this comment.
Nice. We use this prepend pattern in a few other repos (usda-esmis-sufia, exhibits-library-cornell-edu), but we add all those prepends in application.rb instead of its own initializer - I like this better!
Key Features Added
1. Search History Enhancements
blacklight-cornell/app/controllers/search_history_controller.rbblacklight-cornell/app/helpers/cornell_search_history_constraints_helper.rbAND,OR, etc.) and boolean values for clarity.2. Search Session Management
blacklight-cornell/app/controllers/catalog_controller.rbq_row) is present.search_field,q, orf) are populated.3. Query Parameter Filtering
blacklight-cornell/app/services/blacklight/search_service.rbq_row,search_field_row, andboolean_row.4. Custom Blacklight Helper
blacklight-cornell/config/initializers/backlight_custom_helper_loader.rb