Conversation
Coverage Report for CI Build 24889604264Coverage decreased (-0.003%) to 98.352%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 4 blocking issues (4 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR expands the search “source tab” navigation so that additional TIMDEX sources each have a dedicated tab, aligning with USE-517’s requirement that all sources appear in at least one tab.
Changes:
- Adds new TIMDEX tabs (
databases,dspace,geodata) to the allowed tab lists and UI navigation. - Adds TIMDEX query
sourceFiltermappings for the new tabs. - Adds user-facing tab description blurbs for the new tabs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/controllers/application_controller_unit_test.rb | Updates expected timdex_tabs/all_tabs to include the new TIMDEX tabs. |
| app/views/search/_source_tabs.html.erb | Adds new “core” navigation tabs for the added TIMDEX sources. |
| app/models/feature.rb | Minor formatting change to the valid feature list (no behavior change). |
| app/helpers/results_helper.rb | Adds tab description blurbs for dspace, geodata, and databases. |
| app/controllers/search_controller.rb | Adds sourceFilter mappings for the new TIMDEX tabs; minor formatting in error links. |
| app/controllers/application_controller.rb | Extends timdex_tabs to include the new TIMDEX tabs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why are these changes being introduced: * All sources should exist in at least one tab Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-517 How does this address that need: * Adds additional tabs for timdex sources * Adds additional blurbs for the new tabs
JPrevost
force-pushed
the
use-517-more-tabs
branch
from
790afc3 to
e8394a5
Compare
We decided we didn't really want this to be enabled by default, but leaving it in with a high max value to make it easy to go back to this in the future if we want it.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
I have a non-blocking request for a different test type, but it isn't something I'm going to frame as a full request for changes - if there's something in this that I'm missing, I'm happy for this to merge as-is (the type of test I'm talking about didn't exist before, so this isn't a regression)
| params[:tab] = 'website' | ||
| description = 'Information about the library: events, news, services, and more' | ||
| assert_equal description, tab_description | ||
| ResultsHelper::TAB_DESCRIPTIONS.each do |tab, expected| |
There was a problem hiding this comment.
I like the efficiency of this test, but I wonder whether we should have a test for the behavior when a value outside that TAB_DESCRIPTIONS construct is presented?
More theoretically, I get that this test makes things more robust because we don't need to manually keep the list of test values in sync with the content of the helper method. This feels like a win - but I'm not sure where the line is where we just end up testing that .each() does what we expect.
There was a problem hiding this comment.
I definitely agree we should confirm something outside of the expected list returns blank. I'll confirm that test exists or add one.
| module ResultsHelper | ||
| # Descriptions for each tab. HTML entries use raw anchor tags (all URLs are | ||
| # hardcoded external links, so no XSS risk) and must be marked html_safe. | ||
| TAB_DESCRIPTIONS = { |
There was a problem hiding this comment.
I like this change in pattern a lot. 🍻
| else | ||
| TAB_DESCRIPTIONS.fetch(params[:tab]) do | ||
| Rails.logger.error "Unknown tab parameter in `tab_description` helper: #{params[:tab]}" | ||
| '' |
There was a problem hiding this comment.
Making sure I understand - this is the default value in case tab_description gets presented a value that we don't expect, right? So if there's a regression or mismatch somehow, we're not presenting gibberish, just an empty string.
There was a problem hiding this comment.
Correct. Previous to this we were inadvertently returning true as the logging statement is apparently truthy.
|
From my perspective, this looks good to go. Having a const to control the max viewable tabs makes sense. Since the truncation of tabs happens on pretty wide screens now, I should probably adjust the Tablet+ breakpoints to have a more traditional dropdown menu for the "more" overflow instead of the full width menu. We can create a follow-up ticket for that though, and definitely shouldn't block things. |
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
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