refinery / refinerycms-page-images

Adds an images tab to your pages so you can format a group of images really nicely on the frontend
104 stars 120 forks source link

Unpermitted params images_attributes resources_attributes #99

Closed mattherick closed 10 years ago

mattherick commented 10 years ago

If I add the refinerycms-page-images to my rails 4 refinery project (currently I am working with all gems from the master) I always get an unpermitted images_attributes if I add a new image in the context of creating or updating a page.

How can this be fixed?

I already tried to override the new_page_params and the page_params method in the Refinery::Admin::PagesController - but it does not work. I also use an modified version of the refinerycms-page-resources gem (fix of dependencies to the refinerycms gem). Here I always get unpermitted resources_attributes.

Anybody an idea how this can be fixed in both gems - ideally directly in both gems - independent from each other, like a new page_extensions_params method which will be merged with the new_page_params and the page_params methods from the Refinery::Admin::PagesController direclty in this controller?

Anybody an idea?

ugisozols commented 10 years ago

@mattherick check out https://github.com/refinery/refinerycms/commit/cd407e30896657d10a04a8b5de036d577508957f which addressed permitting extra params.

refinerycms-page-images could use a decorator to permit extra params.

mattherick commented 10 years ago

thx! works perfect.

ugisozols commented 10 years ago

:+1:

amdest commented 10 years ago

I'm sorry, guys, i'm totally confused.

Out of the box refinerycms-page-images gives me the following notice, when I try to add an image: out_of_the_box

After I add a pages_controller_decorator as shown here — I get this: with_decorator

Despite that second warning — all listed attributes are successfully saved upon change, except images_attributes.

Rails 4.1.4, Ruby 2.1.2, Refinery from master branch, page-images from master branch.

Thanks.

parndt commented 10 years ago

cc @anitagraham

anitagraham commented 10 years ago

The message (more informational than error) comes whenever you permit fewer attributes than there are in params. I think you just need to put up with it, once you have permitted all the attributes you need. If

params: {a:1, b:2, c:3}
params.permit(:a)
params.permit(:b)

I think you will see two messages

unpermitted parameters: b, c
unpermitted parameters: a, c

and you will be able to update a and b, but not c.

I am just about to add page-images to my sample app (now working for testimonials, thanks @parndt , @gwagener ), so I will watch out for images_attributes.

anitagraham commented 10 years ago

Those images_attributes are tricksy. You would think that you would permit them in the same way that you do parts_attributes.

      def page_params
        params.require(:page).permit(:browser_title, :draft, :link_url, :menu_title, :meta_description,
          :parent_id, :skip_to_first_child, :show_in_menu, :title, :view_template,
          :layout_template, :custom_slug, parts_attributes: [:id, :title, :body, :position])
      end

However doing this:

Refinery::Admin::PagesController.class_eval do

  # Add :images_attributes to page_params for strong parameters.
  def page_params_with_images_attributes_params
    imagesattributes_params = params.require(:page).permit(images_attributes: [:id, :caption])
    page_params_without_images_attributes_params.merge(imagesattributes_params)
  end
  alias_method_chain :page_params, :images_attributes_params

end

Doesn't save the image attributes. It does report

Unpermitted parameters: -1, 0, 1
Unpermitted parameters: title, parts_attributes, draft, parent_id, view_template, menu_title, skip_to_first_child, link_url, show_in_menu, browser_title, meta_descri
ption
Unpermitted parameters: images_attributes

For the record the relevant params are

  "page"=>{
    "title"=>"Images",
    "parts_attributes"=>{
      "0"=>{ "body"=>"<p>Here are some images from my brother's flickr stream.</p>",  "position"=>"0",  "id"=>"1"},
      "1"=>{"body"=>"",  "position"=>"1",  "id"=>"2"}
      },
    "images_attributes"=>{
      "-1"=>"",
      "0"=>{"id"=>"5",   "caption"=>"" },
      "1"=>{"id"=>"3",   "caption"=>""}
      },

parts_attributes and images_attributes should be treated in the same way. (Except perhaps for that -1)

amdest commented 10 years ago

Exactly.

Someone here already mentioned that this empty "-1" key doesn't allow saving.

By the way, when pages_controller_decorator with above mentioned code exist upon server start — the first save of the page that contains PageImage gives "stack level too deep" error. If I comment out page_params_with_images_attributes_params (or remove decorator) without server restart — I get "Unpermitted parameters: images_attributes".

"Stack level too deep" doesn't reappear if i get decorator back without server restart.

anitagraham commented 10 years ago

I am seeing some intermittent successes. Right now and with the following two decorators I can save both images (and testimonials) to a page - if I delete the -1 element from the page before I save.

It is several years since I dug deep into page_image_picker.js so I don't remember the function of the -1 element. I will look at page_resource_picker.js too (which I wrote) and see if I left any hints there.

My real problem is with intermittent. The decorators start with a stack level too deep error. Fiddling around with debug/ap statements or simple variable name changes may "fix" the problem, but my faith in that is not strong.

During the course of a save I see unpermitted parameters:

Unpermitted parameters: title, parts_attributes, images_attributes, draft, parent_id, view_template, menu_title, skip_to_first_child, link_url, show_in_menu, browser
_title, meta_description
Unpermitted parameters: images_attributes, testimonials_show, testimonials_count, testimonials_select
Unpermitted parameters: -1 *if I haven't deleted -1*
Unpermitted parameters: title, parts_attributes, draft, parent_id, view_template, menu_title, skip_to_first_child, link_url, show_in_menu, browser_title, meta_descri
ption, testimonials_show, testimonials_count, testimonials_select
  1. Must be when we permit (or check) the testimonials attributes (which are 3 scalars)
  2. Looks the like the standard permit from page_controller (all the standard attributes)
  3. Anyone's guess
  4. From the permit (or check) of the image_attributes.

Finally the currently working decorators:

Refinery::Admin::PagesController.class_eval do

  # Add :testimonials_attributes to page_params for strong parameters.
  def page_params_with_testimonials_params
    test_params = params.require(:page).permit(:testimonials_show, :testimonials_count, :testimonials_select)
    page_params_without_testimonials_params.merge(test_params)
  end
  alias_method_chain :page_params, :testimonials_params

end

Refinery::Admin::PagesController.class_eval do

  # Add :images_attributes to page_params for strong parameters.
  def page_params_with_images_attributes_params
    page_params_without_images_attributes_params.merge(params.require(:page).permit(images_attributes: [:id, :caption]))
  end
  alias_method_chain :page_params, :images_attributes_params

end
amdest commented 10 years ago

@anitagraham — about "-1" take a look at #100, please.

anitagraham commented 10 years ago

Thanks, looking at the code in the extension that deals with it now.

anitagraham commented 10 years ago

So, it seems the problem is the -1, but we can get it accepted by strong parameters by formatting it properly. It might be time to run some tests.

  <%= hidden_field_tag "#{f.object_name.demodulize}[images_attributes][-1][image_page_id]", "-1" %>
  <%= hidden_field_tag "#{f.object_name.demodulize}[images_attributes][-1][id]", "-1" %>
  <%= hidden_field_tag "#{f.object_name.demodulize}[images_attributes][-1][caption]", "" %>
anitagraham commented 10 years ago

Tests pass, but still getting stack-level-too-deep most of the time, with the occasional success.

anitagraham commented 10 years ago

Final result for a consistently working decorator is

#  http://nirvdrum.com/2009/05/15/alias-method-chain-closure-issue.html
# app/decorators/controllers/admin_pages_controller_decorator.rb
Refinery::Admin::PagesController.class_eval do
  pp_method_builder = Proc.new do
    # Get a reference to the  original method with all previous permissions already applied.
    page_params_method = instance_method :page_params

    # Define the new method.
    define_method("page_params_with_page_image_params") do
      pi_params = params.require(:page).permit(images_attributes: [:id, :caption, :image_page_id])
      page_params_method.bind(self).call().merge(pi_params)
    end
  end

  alias_method_chain :page_params, :page_image_params, &pp_method_builder
end

To handle the -1 parameter (as written above)

#lib/refinery/page-images/extension.rb
....
        module_eval do
          def images_attributes=(data)
            data.reject! {|_, d| d['image_page_id']=='-1'}
            ids_to_keep = data.map{|_, d| d['image_page_id']}.compact