Adds Primo API-based tabs #286
Conversation
Why are these changes being introduced: * Breaking out Primo results into Articles (CDI) and Catalog (Alma) tabs is how we want to present Primo results to users when they are not using the All tab Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-202 How does this address that need: * Adds new tabs for Articles and Catalog in the search results UI * Creates Feature flag `primo_tabs` to enable/disable full Primo results * Modifies PrimoSearch to accept a new `tab` parameter to set the scope * Adds documentation as to what Primo Scopes and Tabs mean and what values are available Document any side effects to this change: * I've moved some Primo Scope and Tab values to code rather than ENV as they are more tightly coupled to the Primo API behavior than ENV implies. Additionally, we use those ENV values to construct links in the UI and those values need to be different than the API calls due to our desire to send users to a specific UI regardless of how we queried the API. The UI values are still set via ENV at this time.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
I have questions about the scope of this change, and whether we're going far enough.
The new tabs seem to work, and the application logic makes sense. I also can see an argument for moving values like scope and tab out of env and into code - but if that's the case, I wonder whether this PR should eliminate them entirely? There are still uses of them in a different model, and they're included in the validate_env method in PrimoSearch.
If there's something I've missed, and those other uses are still valid, I'm open to being shown that.
(I also raise a separate question about how we're handling the tab parameter throughout the application, but that's not something I think is in scope for this PR - so that's much more of a question-leading-to-future-ticket thing)
|
|
||
| Rails.cache.fetch("#{cache_key}/primo", expires_in: 12.hours) do | ||
| primo_search = PrimoSearch.new | ||
| primo_search = PrimoSearch.new(@enhanced_query[:tab]) |
There was a problem hiding this comment.
I don't think this is something to change in this PR, but while looking at this change I want to confirm that this is the only place that consumes the :tab attribute in the enhanced query? A few places look directly to the params object, and that's where the enhancer model gets this value from - but it feels a little awkward how the parameter gets used at various places at the moment.
I don't want to back into any normalization work for this parameter, though, so this isn't a requested change - more of a "does it look this way to you too?" with the idea of having a future ticket to rationalize it.
There was a problem hiding this comment.
I'm open to reflecting on this (not in this ticket though).
I considered a larger refactor by passing the entire enhanced_query into the initialization rather than just the term that gets passed into the search method but that felt too big to do as part of introducing functionality changes.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
This looks good to me - I can turn on and off the various tabs, they seem to work as expected, and the application still works as I expect when I turn on the Geodata feature (and turn off the other tabs - although I'm pretty sure it'd be robust to still having them defined).
Thanks! 
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
primo_tabsto enable/disable full Primo resultstabparameter to set the scopeDocument any side effects to this change:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing