Skip to content
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

Fix combining search filters and pagination #2978

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jul 30, 2024

The search_filter_params method in Alchemy's Resource Controller converts the permitted attributes to a Hash, stopping them from being recognized as something like a nested Hash in our per page select. This commit uses duck-typing in order to find out whether we're looking at a nested Hash. This should work with both ActionController::Parameters and with a Hash.

Unverified

No user is associated with the committer email.
The `search_filter_params` method in Alchemy's Resource Controller
converts the permitted attributes to a Hash, stopping them from being
recognized as something like a nested Hash in our per page select. This
commit uses duck-typing in order to find out whether we're looking at a
nested Hash. This should work with both ActionController::Parameters and
with a Hash.
@mamhoff mamhoff requested a review from a team as a code owner July 30, 2024 14:53
mamhoff added a commit to mamhoff/alchemy_cms that referenced this pull request Jul 30, 2024
When using custom Ransack filters, this will allow combining them with
the pagination. Without this, we can only combine the one ransack field
we use for searching in the top bar.

See AlchemyCMS#2978 for context.
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (e5f8d3c) to head (e059c80).
Report is 127 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2978      +/-   ##
==========================================
+ Coverage   95.96%   96.02%   +0.05%     
==========================================
  Files         232      233       +1     
  Lines        6273     6309      +36     
==========================================
+ Hits         6020     6058      +38     
+ Misses        253      251       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvdeyen tvdeyen added backport-to-7.0-stable Needs to be backported to 7.0-stable backport-to-6.1-stable backport-to-7.1-stable Needs to be backported to 7.1-stable backport-to-7.2-stable Needs to be backported to 7.2-stable labels Jul 30, 2024
@tvdeyen tvdeyen merged commit 1148742 into AlchemyCMS:main Jul 30, 2024
40 checks passed
@alchemycms-bot
Copy link
Collaborator

💔 Some backports could not be created

Status Branch Result
7.0-stable
6.1-stable Backport failed because of merge conflicts
7.1-stable
7.2-stable

Manual backport

To create the backport manually run:

backport --pr 2978

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

mamhoff added a commit to mamhoff/alchemy_cms that referenced this pull request Jul 30, 2024
When using custom Ransack filters, this will allow combining them with
the pagination. Without this, we can only combine the one ransack field
we use for searching in the top bar.

See AlchemyCMS#2978 for context.
mamhoff added a commit to mamhoff/alchemy_cms that referenced this pull request Aug 1, 2024
When using custom Ransack filters, this will allow combining them with
the pagination. Without this, we can only combine the one ransack field
we use for searching in the top bar.

See AlchemyCMS#2978 for context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-7.0-stable Needs to be backported to 7.0-stable backport-to-7.1-stable Needs to be backported to 7.1-stable backport-to-7.2-stable Needs to be backported to 7.2-stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants