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

setting json solr path to nil to prevent browse category query from s… #3137

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

hudajkhan
Copy link
Contributor

@hudajkhan hudajkhan commented Sep 20, 2024

Context
Relates to having tests pass with Blacklight 8.3.
Refer to #3047 and this comment which includes a checklist of tests to fix: #3047 (comment)

For 8.3, the following test is failing:
rspec ./spec/features/browse_category_spec.rb:163 # Browse pages with a search field based browse category conducts a search within the browse category

What this pull request does:

  • Sets json_solr_path property to nil because Blacklight 8 sets the default value to "advanced". (see https://github.com/projectblacklight/blacklight/blob/main/lib/blacklight/configuration.rb#L77). The following code creates the actual query and decides the path: https://github.com/projectblacklight/blacklight/blob/main/lib/blacklight/solr/repository.rb#L136 . With json_solr_path set to "advanced", and given that the browse category rspec query does translate to including a json component (i.e. {"query"=>{"bool"=>{"must"=>[{:edismax=>{:query=>"model"}}, {:edismax=>{:query=>"azimuthal"}}]}}} ), with Blacklight 8, the search path would add "advanced" to the Solr path where Spotlight has no configuration set for an advanced search handler. In Blacklight 7, the solr path used was the regular select. Setting "json_solr_path" to nil fixes this issue.
  • Blacklight 8 also is more strict about passing parameters through to the Solr query. There has already been an update to add "browse_category_id" to the allowed parameters for Blacklight 8. I added it also to the Blacklight 7 list (since it shouldn't break anything and just appears consistent).
  • I left a comment for the browse controller "show" method. The "search_service.search_results" method returns only @response in Blacklight 8 and sets @document_list to nil as a result. In Blacklight 7, both "@response" and "@document_list" is required for the return values for the "search_service.search_results" method to work correctly. I left a comment there to explain what needs to be done there when we move to Blacklight 8.

How to test
These tests should still pass in Blacklight 7.

How to test with Blacklight 8: Generate test app (.internal_test_app). Update the internal test app's Gemfile to use Blacklight 8.3. Run bundle install for the internal test app. Then run rspec ./spec/features/browse_category_spec.rb:163.

@hudajkhan hudajkhan marked this pull request as ready for review September 20, 2024 16:14
@hudajkhan hudajkhan mentioned this pull request Sep 20, 2024
8 tasks
@cbeer
Copy link
Member

cbeer commented Sep 20, 2024

I'm not sure I understand the upstream work, but this seems like a reasonable default for now 🤷‍♂️

@cbeer cbeer merged commit cda1597 into main Sep 20, 2024
7 checks passed
@cbeer cbeer deleted the bl8browse branch September 20, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Blacklight 8
2 participants