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

Don't delete :if and :unless from filters #2523

Merged
merged 1 commit into from
Oct 3, 2013

Conversation

PChambino
Copy link
Contributor

Filter :if and :unless options were being deleted and thus only used for the first request.

@seanlinsley
Copy link
Contributor

What version of AA are you using?

@seanlinsley
Copy link
Contributor

Scratch that; I can reproduce locally

@seanlinsley
Copy link
Contributor

I think this would be a better solution:

diff --git a/lib/active_admin/filters/resource_extension.rb b/lib/active_admin/filters/resource_extension.rb
index d348083..c0eac93 100644
--- a/lib/active_admin/filters/resource_extension.rb
+++ b/lib/active_admin/filters/resource_extension.rb
@@ -76,7 +76,7 @@ module ActiveAdmin
       # Collapses the waveform, if you will, of which filters should be displayed.
       # Removes filters and adds in default filters as desired.
       def filter_lookup
-        filters = @filters.try(:dup) || {}
+        filters = @filters.try(:deep_dup) || {}

Since the whole point of using delete was to pass a clean set of options down the line.

@PChambino
Copy link
Contributor Author

Yes, that looks like a better solution then.

@seanlinsley
Copy link
Contributor

Could you add a test to confirm the bug is fixed?

@PChambino
Copy link
Contributor Author

Sure. I will look into that.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 809630a496fa292768ee9ccbc77d5a971be69be5 on PChambino:patch-1 into 82ff48d on gregbell:master.

@seanlinsley
Copy link
Contributor

Looks good, thanks. One more thing: can you squash this into a single commit with a good commit message?

Filter :if and :unless options were being deleted and thus only
used for the first request.

Deep copy of the filters hash allow safe deletion of filters' options
like :if and :unless, to pass a clean set of options down the line.
@PChambino
Copy link
Contributor Author

Is something like this ok?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ab519e6 on PChambino:patch-1 into 8fa7914 on gregbell:master.

@seanlinsley
Copy link
Contributor

Perfect. Thanks for the contribution!

seanlinsley added a commit that referenced this pull request Oct 3, 2013
Don't delete :if and :unless from filters
@seanlinsley seanlinsley merged commit 7f3d882 into activeadmin:master Oct 3, 2013
seanlinsley added a commit that referenced this pull request Dec 29, 2013
…eted

This was spurred on by #2832, and effectively replaces #2523.

TL;DR it was the filter form's problem, so the resource extension
shouldn't have been trying to compensate.
seanlinsley added a commit that referenced this pull request Dec 29, 2013
This a backport of 4dd5b10 from master to 0-6-stable

Ref: #2523, #2832
seanlinsley added a commit that referenced this pull request Dec 30, 2013
seanlinsley added a commit that referenced this pull request Dec 30, 2013
@seanlinsley seanlinsley mentioned this pull request May 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants