-
Notifications
You must be signed in to change notification settings - Fork 54
Max filters #2195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Max filters #2195
Conversation
…lters has been reached and nobody is logged in #2071
removed in the reverse order the params, not necessarily the order they were added. But the UI shouldn't allow extra params so will remove those added to the end of the url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a mechanism to hide excessive filter options on index pages when the maximum number of filters is reached, unless the user is logged in and registered, and removes any manually added excess filters from the URL.
- Added tests to verify filtering functionality and excess filter removal.
- Implemented helper methods and view changes to control filter display and add warning messages.
- Updated configuration and admin settings to include a new max_filters setting, and removed an unused test file.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/unit/helpers/filtering_helper_test.rb | New tests for max_filters_met? functionality. |
test/unit/helpers/fail_helper_test.rb | Removed an unused test file. |
test/functional/documents_controller_test.rb | Added tests to confirm filter removal and warning message display based on user state. |
lib/seek/index_pager.rb | Added logic to remove excess filters for non-registered users. |
lib/seek/config_setting_attributes.yml | Added max_filters configuration with integer conversion. |
config/initializers/seek_configuration.rb | Set a default value for max_filters. |
app/views/general/_index.html.erb | Updated to conditionally render filter options and warning messages. |
app/views/admin/features_enabled.html.erb | Enhanced admin settings with onchange behavior for filtering settings. |
app/models/user.rb | Updated logged_in_and_registered? logic using registration_complete?. |
app/helpers/filtering_helper.rb | Added a helper method to determine if the maximum filter count is met. |
app/controllers/admin_controller.rb | Updated to assign the max_filters configuration from admin parameters. |
copilot nickpicking Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements logic to hide filter options when the maximum number of filters has been reached (except for registered, logged‐in users) and cleans up excess filters manually added via URL parameters.
- Added tests verifying the new filtering behavior and excess filter removal.
- Introduced helper logic and view updates to conditionally render the filtering UI, including displaying a warning message when the filter limit is exceeded.
- Updated admin settings to allow configuration of the maximum filter limit.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/unit/helpers/filtering_helper_test.rb | Added tests for max_filters_met? method and various filter scenarios. |
test/unit/helpers/fail_helper_test.rb | Removed unused test file for FailHelper. |
test/functional/documents_controller_test.rb | Updated tests for filter display and warning messages under max filter limit. |
lib/seek/index_pager.rb | Added method to remove excess filters when the limit is exceeded. |
lib/seek/config_setting_attributes.yml | Added configuration for max_filters with conversion to integer. |
config/initializers/seek_configuration.rb | Set default value for max_filters. |
app/views/general/_index.html.erb | Updated view to conditionally render filtering UI or warning message. |
app/views/admin/features_enabled.html.erb | Updated admin interface to configure max_filters and show additional settings. |
app/models/user.rb | Updated user logic for determining registration completeness. |
app/helpers/filtering_helper.rb | Provided new helper method max_filters_met? with updated registration check. |
app/controllers/admin_controller.rb | Updated feature configuration for max_filters from admin parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment, otherwise looks good
app/views/general/_index.html.erb
Outdated
<p> | ||
You will need to remove a filter to be able to apply a different one. | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it also suggest registering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered this when adding the message, but was worried about encouraging bots to create fake accounts. But actually, I think it would be better if it informed genuine users so will update.
Hide the filter options if max filters has been reached, unless the user is registered and logged in. Also show a warning explaining why.
Also remove excess filters, should they have been manually added to the URL.
Doesn't affect the API
applied to seek-1.16 for quicker deployment to the Fairdomhub