Add new search tabs and improve tab handling #278

JPrevost · 2025-11-18T16:12:37Z

Why are these changes being introduced:

website and aspace tabs were needed in the search interface to allow users to filter results by source
The timdex tab is not needed for user facing interfaces, but is useful for internal testing and development, so a feature flag was added to control its visibility
The timdex_alma tab will be useful to help us decide if we want a TIMDEX or Primo based Alma tab

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/USE-187

How does this address that need:

Introduces 'website' and 'aspace' tabs to the search interface
refactors tab logic to support dynamic tab lists
adds a feature flag for displaying the TIMDEX tab
Updates tab rendering, controller logic, and tests to accommodate new tabs and feature flag.
Adds support for tabs with spaces in their names
Adds support for tab labels that differ from their internal names which will be important to allow UXWS to change labels without having to update valid tab lists in the code.

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

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

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2025-11-18T16:14:19Z

Pull Request Test Coverage Report for Build 19513403653

Details

22 of 22 (100.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 98.01%

Totals
Change from base Build 19478505178:	0.02%
Covered Lines:	936
Relevant Lines:	955

💛 - Coveralls

Why are these changes being introduced: * `website` and `aspace` tabs were needed in the search interface to allow users to filter results by source * The `timdex` tab is not needed for user facing interfaces, but is useful for internal testing and development, so a feature flag was added to control its visibility * The `timdex_alma` tab will be useful to help us decide if we want a TIMDEX or Primo based Alma tab Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-187 How does this address that need: * Introduces 'website' and 'aspace' tabs to the search interface * refactors tab logic to support dynamic tab lists * adds a feature flag for displaying the TIMDEX tab * Updates tab rendering, controller logic, and tests to accommodate new tabs and feature flag. * Adds support for tabs with spaces in their names * Adds support for tab labels that differ from their internal names which will be important to allow UXWS to change labels without having to update valid tab lists in the code.

matt-bernhardt

I think most of this is fine - but I wonder whether the case block in the results view template needs to list it's all block first, like you've done in the search controller.

Thanks - I also appreciate your having written the method documentation for the link_to_tab helper.

matt-bernhardt · 2025-11-18T20:05:25Z

app/helpers/search_helper.rb

+  # Generates a link for a search results tab, marking it as active if it matches the current active tab.
+  # @param target [String] The name of the tab to link to
+  # @param label [String] The display label for the tab (optional)
+  # @return [String] HTML link element for the tab


Thanks for writing the document block for this helper!

matt-bernhardt · 2025-11-18T20:13:10Z

app/helpers/search_helper.rb

+  # @param target [String] The name of the tab to link to
+  # @param label [String] The display label for the tab (optional)
+  # @return [String] HTML link element for the tab
+  def link_to_tab(target, label = nil)


I wondered how soon we'd need to have a label argument in this helper - I guess the answer is "a few days" :-)

Question:
As I follow the interactions between label and target inside the method, I wonder whether we need tests for this behavior. If I'm following this correctly, we derive clean_target for use in the DOM first, and then overlay label atop target, so that target ends up getting used in human-facing contexts like the visible text on the UI. I like that logic and sequence, so I don't have a concern there - but after dealing with UI bugs all day I'm starting to wonder if we need more tests for this type of thing.

Oh...my names for all of this is confusing after me reading your summary of what is going on :)

I have no idea why I redefine target as clean_target and then create a new target for the label. I'm going to clean that logic up. I think it works as expected but apologies for having to parse wtf is going on.

My fix is likely to change target = label || target to label = label || target so we can use the label throughout. I think the current state was a lazy refactor I did when I slipped in the label feature to this PR in anticipation of UX not actually wanting any of the tab names I was using and not wanting to have to fix a bunch of tests later :)

I've added tests and updated variable names to be clearer.

Ah, I like these variable names better - thanks!

matt-bernhardt · 2025-11-18T20:27:52Z

app/views/search/results.html.erb

-              <% when 'timdex' %>
+              <% when *@timdex_tabs %>
                <%= render(partial: 'search/result', collection: @results, as: :result) %>
              <% when 'all' %>


Requested change:
Like in the search controller above, I think what we need here is to list this block first in the list? Right now, it looks like the Primo partial is getting used to render all results on the all tab, which would be consistent with this block never getting chosen.

Oh... You are correct. I think I need to revisit including all in both lists. It made something easier along the way but you caught an important bug here. That said... since it works I'm not sure why we even have two different view partials for timdex and primo as we already normalized them by this point. We'll see if the work Dave is doing this sprint will leave us needing both or if they'll be combinable.

I'm going to have a few changes to make to fix this though. Thanks for catching it!

Ah, I missed the block under this that has the specific all handling. I don't love that we repeat the partial includes here and am going to rework it as I don't think we need both.

I've reworked this logic to be simpler. There is a side effect that if the result is not either timdex or primo nothing is displayed... but we have tests of the new logic that sets these so the case should never happen. We could decide to change the condition to just be if and then else the other option but I decided to go in this direction. I'm open to reworking.

app/controllers/search_controller.rb

* Adds a lot more tests of the application controller logic by introducing unit tests in addition to Integration tests * Adds an `api` key to both normalized timdex and primo records that we can use in the views * Removes redundant logic for `all` tab in views and normalizes to use newly introduced `api` key * Ensures timdex queries always run when they should be removing condition that unnecessarily complicated all tab queries

matt-bernhardt

This resolves the question I had about handling the all tab in the view, and I think I really like the structure you've come up with - the view is a lot simpler now, which makes me happy.

My questions are about comments and instance variables that may be outdated now - but neither are blocking a merge if there's some aspect of this that I'm missing.

I also have a side question about whether I missed a bug in my earlier review, which you seem to have caught in the latest commits.

Thanks for this!

matt-bernhardt · 2025-11-19T15:15:45Z

app/controllers/search_controller.rb

    # Route to appropriate search based on active tab
    case @active_tab
-    when 'primo'
+    when 'all' # all tab must come first to avoid matching primo or timdex arrays which contain all


Question:
Does the comment here need to change, now that we've removed 'all' from the two tab lists?

Yes, the comment should and will be removed. Great catch. The order here should no longer matter.

matt-bernhardt · 2025-11-19T15:19:54Z

app/controllers/search_controller.rb

      raw = if Feature.enabled?(:geodata)
              execute_geospatial_query(query)
-            elsif @active_tab == 'timdex' || @active_tab == 'all'
+            else


Was this a bug that slipped past me? Reading it now, it seems like raw was going to be empty when the active tab was something like website.

I think this is cleaner, but I want to know if I missed something here before.

I think there was an intermediary change that used the timdex_tabs you may not be seeing if you looked at the full changes rather than just the commit that changed the behavior. It was working on previous commits, but would no longer have handled the all tab case as I removed all from timdex_tabs along the way

matt-bernhardt · 2025-11-19T15:21:33Z

app/helpers/search_helper.rb

+  # @param target [String] The name of the tab to link to
+  # @param label [String] The display label for the tab (optional)
+  # @return [String] HTML link element for the tab
+  def link_to_tab(target, label = nil)


Ah, I like these variable names better - thanks!

matt-bernhardt · 2025-11-19T15:41:17Z

app/controllers/search_controller.rb

-
+
+    @primo_tabs = primo_tabs
+    @timdex_tabs = timdex_tabs


Question:
Do we still need these instance variables? They were used before during the rendering of results, but now that we're handling the rendering based on the new api value in the data, I don't see anywhere in the codebase that still uses these.

This is non-blocking in case I'm missing something, or if there's a longer term plan for these variables that I'm forgetting.

Good catch. Will fix to avoid future confusion.

mitlib temporarily deployed to timdex-ui-pi-use-187-ti-2wfk3s November 18, 2025 16:17 Inactive

JPrevost temporarily deployed to timdex-ui-pi-use-187-ti-2wfk3s November 18, 2025 16:22 Inactive

JPrevost requested review from jazairi and matt-bernhardt November 18, 2025 16:45

matt-bernhardt self-assigned this Nov 18, 2025

JPrevost added 2 commits November 18, 2025 15:25

Update Primo tab display name

8290364

matt-bernhardt requested changes Nov 18, 2025

View reviewed changes

JPrevost force-pushed the use-187-timdex-tabs branch from 28f01d2 to 5ece9e6 Compare November 19, 2025 13:55

JPrevost temporarily deployed to timdex-ui-pi-use-187-ti-2wfk3s November 19, 2025 13:55 Inactive

Update link_to_tab variable names and add tests

9d53faa

JPrevost temporarily deployed to timdex-ui-pi-use-187-ti-2wfk3s November 19, 2025 14:24 Inactive

JPrevost requested a review from matt-bernhardt November 19, 2025 14:26

matt-bernhardt approved these changes Nov 19, 2025

View reviewed changes

Remove unused instance vars and incorrect comment

2d23db5

JPrevost temporarily deployed to timdex-ui-pi-use-187-ti-2wfk3s November 19, 2025 19:12 Inactive

JPrevost merged commit b1c36d2 into main Nov 19, 2025
5 checks passed

JPrevost deleted the use-187-timdex-tabs branch November 19, 2025 19:16

Add new search tabs and improve tab handling #278

Add new search tabs and improve tab handling #278

Uh oh!

Conversation

JPrevost commented Nov 18, 2025

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19513403653

Details

💛 - Coveralls

Uh oh!

matt-bernhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matt-bernhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Nov 18, 2025 •

edited

Loading