Skip to content

Make specialists, facets, and holdings data optional via settings#291

Merged
bertrama merged 10 commits intomainfrom
copilot/make-specialists-facets-optional
Jan 22, 2026
Merged

Make specialists, facets, and holdings data optional via settings#291
bertrama merged 10 commits intomainfrom
copilot/make-specialists-facets-optional

Conversation

Copy link
Contributor

Copilot AI commented Jan 22, 2026

  • Add retrieve_specialists?, retrieve_facets?, and retrieve_holdings? accessor methods to Spectrum::Request::Requesty module
  • Update Primo search engine to conditionally request facets based on retrieve_facets?
  • Update Solr search engine to conditionally request facets based on retrieve_facets?
  • Update search.rb to conditionally fetch specialists based on retrieve_specialists?
  • Update search.rb to conditionally fetch holdings based on retrieve_holdings?
  • Add tests for the new accessor methods
  • Code review completed with no issues
  • Security scan completed with no vulnerabilities
  • Fixed Primo implementation per feedback - using disableSplitFacets parameter instead of modifying extract_facets
  • Corrected disableSplitFacets to use request.retrieve_facets? directly (not inverted)
  • Added stub methods to Spectrum::Request::Null

Summary

All changes have been implemented successfully. The implementation allows clients to optionally disable specialists, facets, and holdings retrieval via the settings object in search requests. All features default to enabled (true) for backward compatibility.

Changes:

  1. Added three new accessor methods in Spectrum::Request::Requesty that check the settings object
  2. Modified Solr facet extraction to respect the retrieve_facets? setting
  3. Updated Primo params method to add disableSplitFacets: request.retrieve_facets? (as specified)
  4. Updated search.rb to conditionally fetch specialists and holdings
  5. Added early return in Specialists class to skip processing when disabled
  6. Created comprehensive test coverage for all three settings
  7. Added stub methods to Spectrum::Request::Null that return true for all three accessor methods

Primo Implementation:

  • Added disableSplitFacets key in params method with value from request.retrieve_facets?
  • disableSplitFacets has the same value as request.retrieve_facets? (not inverted)
  • extract_facets properly handles user-selected facets as intended
Original prompt

This section details on the original issue you should resolve

<issue_title>Make specialists, facets, and realtime holdings data optional</issue_title>
<issue_description>Make specialists, facets, and holdings data optional.

The preference will be set via the settings key for search requests.
The request object specification is:

{
  'uid': STRING
    // A machine readable identifier for the datastore you are querying.

  'request_id': ANYTHING
    // Some data that the server will not modify.
    // The 'request' object in the response will have an exact copy of what was
    // sent in this field.
    // The 'new_request' object in the response will not contain this value
    // because the 'new_request' object may not be related to this search such
    // as in the case of a multisearch search.

  'start': INT
    // The index of the item you want to start with. 0 is the first index.

  'count': INT
    // The number of results you are requesting.
    // Note: the server should never send more results than the client
    // requested. However, the server may send fewer results than requested.

  'field_tree': OBJECT
    // A tree that describes the fields being searched, the values to be placed
    // into those fields, and how to combine them together.
    // An empty object should be sent when not sending any values.
    // See below for how this tree is formatted
    
  'raw_query': STRING
    // A string representing the raw query.  Client-side parsing is being
    // phased out in favor of server-side parsing.  The server-side parser
    // is currently (9/28/2022) used for a limited number of datastores.

  'facets': OBJECT
    // An object which contains the facets that should be applied to the search.
    // Each facet is represented as a key/value pair, where the key is the 'uid'
    // of the facet as defined by the datastore object, and the value is an
    // array of whatever facet values have been selected by the user. The facet
    // values can be anything (boolean, string, object, array, number, etc).

  'sort': STRING
    // The 'uid' for the sort you want to use.

  'settings': OBJECT
    // An object of additional settings for how you want results formatted.
    // Each settings should be set as a key/value pair, where the key is the
    // 'uid' for that setting and the value is whatever you want to set it to.
}

And an example would be:

{
  "count": 10,
  "uid": "catalog",
  "start": 0,
  "request_id": 1,
  "field_tree": {},
  "raw_query": "Stephen King",
  "facets": {},
  "sort": "relevance",
  "settings": {
    "specialists": false,
    "facets": false,
    "holdings": false
  }
}

The module Spectrum::Request::Requesty will need to have an accessor function for retrieve_specialists?, retrieve_facets?, and retrieve_holdings?. These default to true unless the respective key in settings is set to false.

Then the Primo and Solr search engines will need to determine whether to request facets in their respective requests.
</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 22, 2026 17:20
…ethods and implement conditional data fetching

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
…oldings? methods

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
Copilot AI changed the title [WIP] Make specialists, facets, and holdings data optional Make specialists, facets, and holdings data optional via settings Jan 22, 2026
Copilot AI requested a review from bertrama January 22, 2026 17:25
@bertrama
Copy link
Member

@copilot The changes to the Primo search engine are incorrect.

extract_facets is about applying user-selected facets to the search query, not whether facets will be retrieved with the search. Revert that change.

A correct implementation will change the params method of the Spectrum::Config::PrimoSource class instead. And will set a hash key called: disableSplitFacets. disableSplitFacets will have the same value as request.retrieve_facets?

…ams method instead of modifying extract_facets

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 22, 2026

@copilot The changes to the Primo search engine are incorrect.

extract_facets is about applying user-selected facets to the search query, not whether facets will be retrieved with the search. Re...

Fixed in commit ba36dea. Reverted the changes to extract_facets and instead added disableSplitFacets: request.retrieve_facets? to the params method as requested.

…ehavior

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
@bertrama
Copy link
Member

@copilot The boolean logic for disableSplitFacets is not inverted.

…ve_facets? directly

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 22, 2026

@copilot The boolean logic for disableSplitFacets is not inverted.

Fixed in commit 8392ce2. Changed disableSplitFacets: !request.retrieve_facets? to disableSplitFacets: request.retrieve_facets? to match the original specification (not inverted).

@bertrama
Copy link
Member

@copilot Add stub methods for Spectrum::Request::Null for the three accessor methods that return true.

…rieve_holdings? to Spectrum::Request::Null

Co-authored-by: bertrama <2932803+bertrama@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 22, 2026

@copilot Add stub methods for Spectrum::Request::Null for the three accessor methods that return true.

Added in commit 7aaacb1. The three methods (retrieve_specialists?, retrieve_facets?, and retrieve_holdings?) now return true in Spectrum::Request::Null.

This chained hash lookup throws an exception when the first one retrieves nil.
@bertrama bertrama marked this pull request as ready for review January 22, 2026 19:23
@bertrama bertrama merged commit 4db0d04 into main Jan 22, 2026
3 checks passed
@bertrama
Copy link
Member

I fixed some issues and reverted an unnecessary change.

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.

Make specialists, facets, and realtime holdings data optional

2 participants