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 regression that caused untranslated labels of boolean filters #2206

Closed

Conversation

dmfrancisco
Copy link
Contributor

This slightly changes 47fe4c5 in order to do the same thing, while translating labels properly. I'm not sure if it's the best approach, so reject this pull request if you have something different in mind 😉

@seanlinsley
Copy link
Contributor

Can you provide a failing test to go along with this? While I didn't check whether 47fe4c5 affected form translations when I wrote it, I don't understand how those changes could have caused this.

@dmfrancisco
Copy link
Contributor Author

Thank you for the quick reply. I added a new test which fails with 47fe4c5.

I think it happens because super is called inside label_text before removing the "_eq", so no translations are found. Since the super method calls method internally, the only way I can think of to prevent this issue is to not append the "_eq" to method.

@seanlinsley
Copy link
Contributor

Ah... Instead of reverting my commit, how about moving the logic I had put in method to input_name instead:

def input_name
  super.to_s =~ search_conditions ? super : "#{super}_eq" 
end

Does that work?

@dmfrancisco
Copy link
Contributor Author

Sadly that breaks two tests, because to_html calls check_box_html which internally calls method, which will return the method name without the "_eq", and metasearch raises an error:

ActiveAdmin::Filters::ViewHelper boolean attribute boolean datatypes should create a check box for equals to
Failure/Error: text_node active_admin_filters_form_for(*assigns[:filter_args])
NoMethodError:
  undefined method `starred' for #<MetaSearch::Searches::Post:0xbb66dbac>
# ./lib/active_admin/inputs/filter_boolean_input.rb:9:in `block in to_html'
# ./lib/active_admin/inputs/filter_base.rb:8:in `input_wrapping'
# ./lib/active_admin/inputs/filter_boolean_input.rb:7:in `to_html'
# ./lib/active_admin/form_builder.rb:19:in `block in input'
# ./lib/active_admin/form_builder.rb:177:in `with_new_form_buffer'
# ./lib/active_admin/form_builder.rb:19:in `input'
# ./lib/active_admin/filters/forms.rb:11:in `filter'

@seanlinsley
Copy link
Contributor

I'll take a closer look at this tomorrow.

On May 17, 2013, at 6:55 PM, David Francisco notifications@github.com wrote:

Sadly that breaks two tests, because to_html calls check_box_html which internally calls method, which will return the method name without the "_eq", and metasearch raises an error:

ActiveAdmin::Filters::ViewHelper boolean attribute boolean datatypes should create a check box for equals to
Failure/Error: text_node active_admin_filters_form_for(*assigns[:filter_args])
NoMethodError:
undefined method `starred' for #MetaSearch::Searches::Post:0xbb66dbac

./lib/active_admin/inputs/filter_boolean_input.rb:9:in `block in to_html'

./lib/active_admin/inputs/filter_base.rb:8:in `input_wrapping'

./lib/active_admin/inputs/filter_boolean_input.rb:7:in `to_html'

./lib/active_admin/form_builder.rb:19:in `block in input'

./lib/active_admin/form_builder.rb:177:in `with_new_form_buffer'

./lib/active_admin/form_builder.rb:19:in `input'

./lib/active_admin/filters/forms.rb:11:in `filter'


Reply to this email directly or view it on GitHub.

@marcusg
Copy link
Contributor

marcusg commented May 21, 2013

i can confirm this bug

@seanlinsley
Copy link
Contributor

Sorry for the delay. This PR looks good. If you can squash these into a single commit I'll merge this in.

If you would, please reference 47fe4c5 from the commit message for traceability.

@dmfrancisco
Copy link
Contributor Author

Ok, thanks!

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.

3 participants