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

Form Builders passes incorrect id placeholder for `has_many` #1832

Closed
inossidabile opened this Issue Dec 30, 2012 · 9 comments

Comments

Projects
None yet
7 participants
@inossidabile

inossidabile commented Dec 30, 2012

https://github.com/gregbell/active_admin/blob/master/lib/active_admin/form_builder.rb#L175 this line makes ActiveAdmin use non-ASCI characters (you can use any within locales, right?) for JS generation.

It leads to string corruption and NEW_*_RECORD stays unreplaced with getTime. This in turn leads to HTML ids duplication.

Class name should be used inside a placeholder instead.

Here is the monkeypatch:

ActiveAdmin::FormBuilder.class_eval do
  def js_for_has_many(association, form_block, template)
    association_reflection = object.class.reflect_on_association(association)
    association_human_name = association_reflection.klass.model_name
    placeholder = "NEW_#{association_human_name.upcase.split(' ').join('_')}_RECORD"

    js = with_new_form_buffer do
      inputs_for_nested_attributes :for => [association, association_reflection.klass.new],
                                   :class => "inputs has_many_fields",
                                   :for_options => { :child_index => placeholder },
                                   &form_block
    end

    js = template.escape_javascript(js)

    text = I18n.t 'active_admin.has_many_new', :model => association_human_name.human
    onclick = "$(this).siblings('li.input').append('#{js}'.replace(/#{placeholder}/g, new Date().getTime())); return false;"

    template.link_to text, "#",
                     :onclick => onclick,
                     :class => "button"
  end

end
@seanlinsley

This comment has been minimized.

Show comment
Hide comment
@seanlinsley

seanlinsley Jan 3, 2013

Member

Just an FYI: replace $(this).siblings('li.input').append( with $(this).before(, otherwise your patch won't work. That change was introduced by a63cd90 into 0.5.1, and it breaks f.has_many.

Member

seanlinsley commented Jan 3, 2013

Just an FYI: replace $(this).siblings('li.input').append( with $(this).before(, otherwise your patch won't work. That change was introduced by a63cd90 into 0.5.1, and it breaks f.has_many.

@Mike-Petersen

This comment has been minimized.

Show comment
Hide comment
@Mike-Petersen

Mike-Petersen Jan 28, 2013

I can confirm the bug in 0.5.1

Mike-Petersen commented Jan 28, 2013

I can confirm the bug in 0.5.1

@ghost ghost assigned seanlinsley Mar 14, 2013

@donbobka

This comment has been minimized.

Show comment
Hide comment
@donbobka

donbobka commented Mar 28, 2013

+1

@Cr1stal

This comment has been minimized.

Show comment
Hide comment
@Cr1stal

Cr1stal commented Mar 28, 2013

+1

@pashpashkin

This comment has been minimized.

Show comment
Hide comment
@pashpashkin

pashpashkin commented Mar 28, 2013

+1

@andrey-lizunov

This comment has been minimized.

Show comment
Hide comment
@andrey-lizunov

andrey-lizunov commented Mar 28, 2013

+1

@seanlinsley

This comment has been minimized.

Show comment
Hide comment
@seanlinsley

seanlinsley Mar 28, 2013

Member

The only real change in the monkeypatch was moving where we call human, which does translation.

-      association_human_name = association_reflection.klass.model_name.human
+      association_human_name = association_reflection.klass.model_name
       placeholder = "NEW_#{association_human_name.upcase.split(' ').join('_')}_RECORD"

       js = with_new_form_buffer do
@@ -183,10 +183,12 @@ module ActiveAdmin

       js = template.escape_javascript(js)

-      text = I18n.t 'active_admin.has_many_new', :model => association_human_name
+      text = I18n.t 'active_admin.has_many_new', :model => association_human_name.human
       onclick = "$(this).before('#{js}'.replace(/#{placeholder}/g, new Date().getTime())); return false;"

-      template.link_to(text, "#", :onclick => onclick, :class => "button").html_safe
+      template.link_to text, "#",
+                       :onclick => onclick,
+                       :class => "button"
     end
Member

seanlinsley commented Mar 28, 2013

The only real change in the monkeypatch was moving where we call human, which does translation.

-      association_human_name = association_reflection.klass.model_name.human
+      association_human_name = association_reflection.klass.model_name
       placeholder = "NEW_#{association_human_name.upcase.split(' ').join('_')}_RECORD"

       js = with_new_form_buffer do
@@ -183,10 +183,12 @@ module ActiveAdmin

       js = template.escape_javascript(js)

-      text = I18n.t 'active_admin.has_many_new', :model => association_human_name
+      text = I18n.t 'active_admin.has_many_new', :model => association_human_name.human
       onclick = "$(this).before('#{js}'.replace(/#{placeholder}/g, new Date().getTime())); return false;"

-      template.link_to(text, "#", :onclick => onclick, :class => "button").html_safe
+      template.link_to text, "#",
+                       :onclick => onclick,
+                       :class => "button"
     end
@donbobka

This comment has been minimized.

Show comment
Hide comment
@donbobka

donbobka Mar 28, 2013

Because of translation in line association_human_name = association_reflection.klass.model_name.human, placeholder variable may consist of non-ASCII symbols(for example: cyrillic).

Result HTML before monkeypatch http://note.io/11Seba4
Result HTML after monkeypatch http://note.io/XeGyBk

donbobka commented Mar 28, 2013

Because of translation in line association_human_name = association_reflection.klass.model_name.human, placeholder variable may consist of non-ASCII symbols(for example: cyrillic).

Result HTML before monkeypatch http://note.io/11Seba4
Result HTML after monkeypatch http://note.io/XeGyBk

@seanlinsley

This comment has been minimized.

Show comment
Hide comment
@seanlinsley

seanlinsley Mar 28, 2013

Member

The problem's been resolved as of 0f51868

Member

seanlinsley commented Mar 28, 2013

The problem's been resolved as of 0f51868

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