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 deleting if and unless options from filters #2832

Closed
wants to merge 1 commit into from

Conversation

Fivell
Copy link
Member

@Fivell Fivell commented Dec 28, 2013

I have next code in one of my AA applications which use dynamic filter using if, unless options

Report::CustomCdr::CDR_COLUMNS.each do |key|
      filter Report::CustomData.normalized_column_name(key.to_s).to_sym, :if => proc{
        @custom_cdr.group_by_include? key
      }
  end

It works , but only first time, after reloading page I all filters are on the sidebar like it was no :if . I started to debug and noticed that after first rendering, if and unless options are not used anymore, because they were deleted in active_admin_filters_form_for method.

@seanlinsley
Copy link
Contributor

Are you sure your application was using the latest code on master? Because I'm almost positive that I've fixed this problem before.

@seanlinsley
Copy link
Contributor

Or, @PChambino did: #2523

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.13%) when pulling 7b07e08 on YETI-switch:fix_filter_options into 5e4fe26 on gregbell:master.

@Fivell
Copy link
Member Author

Fivell commented Dec 28, 2013

@seanlinsley , it is almost the same as #2523 , but fix #2523 can't be backported to 0.6.3 and can be used only with rails4 because Array#deep_dup was added in rails4, rails/rails@657b4ff

@seanlinsley
Copy link
Contributor

Array#deep_dup was introduced in Rails 4, but Hash#deep_dup (which is what #2523 uses) is at the very least available in Rails 3.2

@Fivell
Copy link
Member Author

Fivell commented Dec 29, 2013

@seanlinsley , if it's Hash then yes it's ok. https://github.com/gregbell/active_admin/blob/master/lib/active_admin/filters/resource_extension.rb#L20 this comment make me thing that it is still array in master.

@Fivell
Copy link
Member Author

Fivell commented Dec 29, 2013

@seanlinsley, if you are going to backport both #2523 and ca92329 to 0.6.3 it is ok for me, so we can close this PR

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
Copy link
Contributor

Turns out #2523 can't be backported for 0.6.3 since Hash#deep_dup was first added to Rails 3.1, and we still need to support Rails 3.0. So instead I'm fixing the problem at the source.

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
@Fivell
Copy link
Member Author

Fivell commented Dec 30, 2013

@seanlinsley , I tested it and I can tell that your commit doesn't help.

@Fivell
Copy link
Member Author

Fivell commented Dec 30, 2013

@seanlinsley your commit (fa0edec) have next lines

return if opts.key?(:if)     && !call_method_or_proc_on(self, opts[:if])
return if opts.key?(:unless) &&  call_method_or_proc_on(self, opts[:unless])

I belive I mean

next if opts.key?(:if)     && !call_method_or_proc_on(self, opts[:if])
next if opts.key?(:unless) &&  call_method_or_proc_on(self, opts[:unless])

So first failed condition turns off all other filters

@seanlinsley
Copy link
Contributor

Ah... thanks for catching that.

(in which I add tests that try adding more than one filter at a time)

@seanlinsley seanlinsley reopened this Dec 30, 2013
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
Copy link
Contributor

Finally done.

@Fivell Fivell deleted the fix_filter_options branch December 30, 2013 18:05
@Fivell Fivell restored the fix_filter_options branch December 30, 2013 18:06
@Fivell Fivell deleted the fix_filter_options branch December 30, 2013 18:06
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