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

Update guide Menu Presenter for Refinery 3.0 #2953

Merged
merged 2 commits into from Apr 24, 2015

Conversation

bricesanchez
Copy link
Member

No description provided.


alias_method_chain :page_params, :show_in_footer_params
end
end
Copy link
Member

Choose a reason for hiding this comment

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

This workaround shouldn't be required since Decorators 2.0.0 was released, and you're ignoring all of the params that were previously allowed. Try this:

def page_params_with_show_in_footer
  page_params_without_show_in_footer.permit(:show_in_footer)
and
alias_method_chain :page_params, :show_in_footer

Copy link
Member

Choose a reason for hiding this comment

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

You mean alias_method_chain?

@bricesanchez
Copy link
Member Author

Fixed, thanks for the tip, i didn't know that the workaround was obsolete.

@bricesanchez bricesanchez added this to the 3.0 milestone Apr 16, 2015
@bricesanchez bricesanchez self-assigned this Apr 16, 2015
@bricesanchez
Copy link
Member Author

As i understood it, your tip doesn't work :

Refinery::Admin::PagesController.class_eval do
  def page_params_with_show_in_secondary_menu
    page_params_without_show_in_secondary_menu.permit(:show_in_secondary_menu)
  end
  alias_method :page_params, :show_in_secondary_menu

  def page_params_with_show_in_main_menu
    page_params_without_show_in_main_menu.permit(:show_in_main_menu)
  end
  alias_method :page_params, :show_in_main_menu

  def page_params_with_show_in_footer_menu
    page_params_without_show_in_footer_menu.permit(:show_in_footer_menu)
  end
  alias_method :page_params, :show_in_footer_menu
end

@bricesanchez
Copy link
Member Author

if i use alias_method_chain it cause a loop error :

SystemStackError (stack level too deep):
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:3:in `page_params_with_show_in_secondary_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:8:in `page_params_with_show_in_main_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:13:in `page_params_with_show_in_footer_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:3:in `page_params_with_show_in_secondary_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:8:in `page_params_with_show_in_main_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:13:in `page_params_with_show_in_footer_menu'
  app/decorators/controllers/refinery/admin/pages_controller_decorator.rb:3:in `page_params_with_show_in_secondary_menu'

Remove workaround for show_in_footer param, useless since decorators 2.0,0

Fix typo in menu decorator. Thanks @anitagraham
@parndt
Copy link
Member

parndt commented Apr 21, 2015

@bricesanchez which version of decorators is your project using?

@bricesanchez
Copy link
Member Author

It uses decorator 2.0.1, i didn't check the syntax of my code today, i will have time tomorrow, i will let you know if i still have this error.

@bricesanchez
Copy link
Member Author

As @anitagraham said on gitter, this snippet works for her and me :

Refinery::Admin::PagesController.class_eval do
  def page_params_with_menus_params
    menus_params = params.require(:page).permit(:show_in_secondary_menu,
                                                :show_in_main_menu,
                                                :show_in_footer_menu)
  end

  alias_method_chain :page_params, :menus_params
end

not something like that:

Refinery::Admin::PagesController.class_eval do
  def page_params_with_show_in_footer
    page_params_without_show_in_footer.permit(:show_in_footer)
  end
  alias_method_chain :page_params, :show_in_footer
end

@parndt
Copy link
Member

parndt commented Apr 22, 2015 via email

@bricesanchez
Copy link
Member Author

I don't know how to check if they are ignored but it looks like to work well.

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Well they will be ignored, because this method you've proposed overrides the original method without calling the original method.

@bricesanchez
Copy link
Member Author

So,what is the good way? I was using the workaround until your advice. I don't know what to think here.

@anitagraham
Copy link
Contributor

I use a merge:

testimonial_params = params.require(:page).permit(:testimonials_show, :testimonials_count, :testimonials_select)
page_params_without_testimonial_params.merge(testimonial_params)

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Ah, you can merge those? I thought you had to call permit again.

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Okay, I've figured out the basic trouble and I will add a patch to this branch.

@parndt
Copy link
Member

parndt commented Apr 23, 2015

@simi @bricesanchez @anitagraham I think it's much nicer now?

@anitagraham
Copy link
Contributor

I'm having trouble getting this to work. Will it work with the application and an extension using this technique?

@parndt
Copy link
Member

parndt commented Apr 23, 2015

It should do, I just tried it and it worked for me. (I know, worst ever response). Let me know what trouble you're having?

@anitagraham
Copy link
Contributor

I can set and unset the page.show_in_footer successfully on its own.

I've added the same pattern to refinerycms-testimonials thus:

module RefineryAdminPageParams
  def permitted_page_params
    super << [:testimonials_show, :testimonials_count, :testimonials_select]
  end
end
Refinery::Admin::PagesController.prepend RefineryAdminPageParams

If I add refinerycms-testimonials to the gemfile (bundle gemfile etc) I can no longer set the page.show_in_footer, and see
Unpermitted parameter: show_in_footer in the log, and the parameter is not set.

Here: (https://gist.github.com/anitagraham/ed3b73e450cffed70fc6#file-show-in-footer-not-working-L21)
and here: (https://gist.github.com/anitagraham/ed3b73e450cffed70fc6#file-show-in-footer-working-L39)

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Is that because you're using the same module name?

@anitagraham
Copy link
Contributor

Probably... but then the example should name its module like this

module RefineryAdminPageShowInFooterParams

(Params optional perhaps)

@parndt
Copy link
Member

parndt commented Apr 23, 2015

@anitagraham I was thinking maybe we should do this instead:

Refinery::Admin::PagesController.prepend(
  Module.new do
    def permitted_page_params
      super << :show_in_footer
    end
  end
)

@anitagraham
Copy link
Contributor

Module.new what happens to Module.old?
(I could look it up but then it makes the discussion less enlightening to those reading it a year later...)

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Haha.. it just creates a new anonymous module with this method and prepends it onto the class, thus making it so that super still works.

@parndt parndt force-pushed the update-guide-menu-presenter branch from f69dd7a to 21b311c Compare April 23, 2015 06:02
@anitagraham
Copy link
Contributor

Hokay. Also did you notice - as I just did looking for Module.new - that the Google Doodle today is Dame Ngaio Marsh? A New Zealand Google doodle.

@parndt
Copy link
Member

parndt commented Apr 23, 2015

Nice!

@anitagraham does this work with your example from before?

@anitagraham
Copy link
Contributor

Yes!! 👍

bricesanchez added a commit that referenced this pull request Apr 24, 2015
Update guide Menu Presenter for Refinery 3.0
@bricesanchez bricesanchez merged commit 9c7a7f2 into master Apr 24, 2015
@bricesanchez bricesanchez deleted the update-guide-menu-presenter branch January 27, 2016 14:37
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

4 participants