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

Change the way of assigning template variables #4633

Merged
merged 1 commit into from Jan 23, 2017

Conversation

chumakoff
Copy link
Contributor

ActionView::Base#assign method removes all previous view variable assigned from ActionView::Base assigns hash if there are any.

For example:

template.assigns # => {page_title: "Awesome Title"}
template.assign(has_many_block: true) # this will clear the previous `assigns` hash
template.assigns # => {has_many_block: true}

This PR changes the way of assigning template variables, to avoid previously assigned values loss:

template.assigns[:has_many_block] = true

One of possible problems that this PR solves (reproduction steps):

  1. Add custom action with a custom title to a controller:
member_action :custom_action do 
  @page_title = "My custom title"
end
  1. Add template for this action that contains a form with :has_many
semantic_form_for [:admin, Article.new], builder: ActiveAdmin::FormBuilder do |f|
  ....
  f.has_many :comments do |coment_form|
    ....
  1. When you open the page, you will not see "My custom title". The reason is that has_many method calls template.assign(...) method, which clears template's assigns hash, so template.assigns[:page_title] returns nil.

Note

This way (provided by this PR) of assigning template variables does not add an instance variable to the ActionView::Base object (template). If it is needed to add an instance variable, the solution should be like this:

template.assigns[:foo] = 'bar'
template.instance_variable_set("@foo", 'bar')

But I think it is not necessary, because in all cases when the assignment is performed the variables are not used as instance variables in the view and are not assigned for this purpose.

Copy link
Contributor

@varyonic varyonic left a comment

Choose a reason for hiding this comment

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

Good. Needs rebase.

@varyonic varyonic added the bug label Jan 21, 2017
@Fivell Fivell assigned Fivell and unassigned Fivell Jan 21, 2017
@Fivell Fivell self-requested a review January 21, 2017 23:51
@codecov-io
Copy link

codecov-io commented Jan 22, 2017

Current coverage is 98.25% (diff: 100%)

Merging #4633 into master will not change coverage

@@             master      #4633   diff @@
==========================================
  Files           144        144          
  Lines          4069       4069          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           3998       3998          
  Misses           71         71          
  Partials          0          0          

Powered by Codecov. Last update a81cd03...64f6099

`ActionView::Base#assign` method removes all previous view variable assignments from ActionView::Base `assigns` hash if there are any.

For example:

    template.assigns # => {page_title: "Awesome Title"}
    template.assign(has_many_block: true) # this will clear the `assigns` hash
    template.assigns # => {has_many_block: true}

This commit changes the way of assigning template variables, to avoid previously assigned values loss:

    template.assigns[:has_many_block] = true
@varyonic varyonic removed the ready label Jan 23, 2017
@varyonic varyonic merged commit c946408 into activeadmin:master Jan 23, 2017
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.

None yet

4 participants