Add :tab and :booleanType to permit call in view #266
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
** Why are these changes being introduced: * We are building links to the different result tabs in the _source_tabs view partial via the params.permit() method, but that list is not comprehensive of the params that the SearchController is then adding. This mismatch between permitted and used params is generating messages in the logs about unpermitted parameters (two of them for every page load, one for each tab) ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-94 ** How does this address that need: * This adds the :tab and :booleanType values to the list of permitted parameters for building those links, which results in those log messages no longer being written. ** Document any side effects to this change: There are a few different ways we might resolve this behavior - I've chosen the simplest to adopt now, but it may not be the best option long term. For example, if next week we expand the SearchController to add another new parameter, that would need to be added to this partial or we'll have another round of log messages. Other options include: - Explicitly building the links by including every relevant parameter, i.e. results_path(q: params[:q], per_page: params[:per_page]... - this would limit the need to keep this partial in sync with the controller. - Choosing a different way of handling values like :booleanType that does not rely on manipulating the params object in the midst of responding to a page request. This is probably the cleanest option, but means refactoring more of the application, and a larger level of effort. Note that I originally tried to resolve this condition via a before_action lifecycle hook in the SearchController, explicitly making :tab and :booleanType params permitted. That doesn't work because the partial also calls .permit, which acts independently of anything the controller does or doesn't do.
Pull Request Test Coverage Report for Build 19178173621Details
💛 - Coveralls |
JPrevost
approved these changes
Nov 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR resolves an issue we've noticed with our log stream, where there are warnings about unpermitted parameters on result page loads. The root cause appears to be that the partial which loads the tab links for USE uses the
params.permit()pattern, while not accounting for the fact that the search controller has added two additional parameters that aren't listed.The simplest solution has been to add those two parameters, which appears to silence the log messages. I've checked this with the application in Geodata mode, and the log messages are likewise absent there - although if I'm right about the root cause, Geodata wouldn't have seen these messages anyway because it doesn't load the tabs partial.
There are other options available for resolving this condition - I came up with two during the course of this work. Each are slightly more work than this, but may be more robust than what I'm proposing. In particular, the approach in this PR requires us to keep the list of parameters in the tabs partial in sync with any parameters that the search controller adds. That coordination issue feels non-ideal, but I want to propose this PR as-is in case I'm overthinking the solution. Our roadmap for this application may not require any further manipulation of the parameters by the search controller.
The commit message has a more thorough exploration of the alternatives, so please check that out if you're curious.
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