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 sorting in Resources controller #1462

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Aug 20, 2018

What is this pull request for?

Fix sorting in resources controller.

Notable changes (remove if none)

Prior to this commit, Ransack sorting would not work
with the standard resources controller, as the parameter
q[s] that is generated by Ransack's sort_link helper
will be thrown away as insecure.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. A minor nit on spec context, that might be worth addressing.

@@ -46,6 +46,15 @@
expect(assigns(:events)).not_to include(lustig)
end
end

context 'with search parameter given' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you meant 'with sort parameter given'

Prior to this commit, Ransack sorting would not work
with the standard resources controller, as the parameter
`q[s]` that is generated by Ransack's `sort_link` helper
will be thrown away as insecure.
@mamhoff mamhoff force-pushed the allow-sort-parameter-in-resource-controller branch from 7224bb4 to ea1b4f0 Compare August 20, 2018 12:19
@tvdeyen tvdeyen merged commit 878ad35 into AlchemyCMS:master Aug 20, 2018
@tvdeyen tvdeyen added this to the 4.1 milestone Sep 10, 2018
@tvdeyen
Copy link
Member

tvdeyen commented Sep 10, 2018

Note to self: Needed because of Ransack 2.0. Works in Ransack 1.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants