Rails hidden ID field causes exception

jvanus commented 6 years ago

This issue defines a narrow case as I ran into it. The proposed fixes try to assume there are other scenarios that could cause the same problem. I think it would be fair to classify this as a feature request rather than a bug, as the problematic html does not conform to the example in README.md. I think this may be a somewhat common use case however so its probably worth 'fixing'. I would be happy to work on a patch for this, but I'd like to know your thoughts before I do.

Problem:

Rails automatically adds hidden fields after form_for or fields_for which may be at the same level rails_sortable is looking for children to submit.

For example: survey.rb

class Survey < ApplicationRecord
  has_many :questions
  ...

surveys/_form.html.erb

<%= form_for(@survey) %>
...
<div class="ui-sortable">
  <%= f.fields_for :questions do |q| %>
    <div id="Question_<%= q.object.id %>" class="ui-sortable-handle">
      ...
    </div>
  <% end %>
</div>

Will produce html like:

<div class="ui-sortable">
  <input type="hidden" value="1" id="survey_questions_attributes_0_id" name="survey[questions_attributes][0][id]">
  <div id="Question_1" class="ui-sortable-handle">
    ...
  </div>
</div>

Stack Trace:

In this case parse_params is expecting the params hash to only have the key Questions.

Started POST "/sortable/reorder" for 127.0.0.1 at 2017-12-15 17:15:55 -0700
Processing by SortableController#reorder as */*
  Parameters: {"survey_questions_attributes_0"=>["id"], "survey_questions_attributes_1"=>["id"], "Question"=>["3", "1"]}
  Completed 500 Internal Server Error in 2ms (ActiveRecord: 0.2ms)

NameError (wrong constant name survey_questions_attributes_0):

activesupport (5.1.2) lib/active_support/inflector/methods.rb:269:in `const_get'
activesupport (5.1.2) lib/active_support/inflector/methods.rb:269:in `block in constantize'
activesupport (5.1.2) lib/active_support/inflector/methods.rb:267:in `each'
activesupport (5.1.2) lib/active_support/inflector/methods.rb:267:in `inject'
activesupport (5.1.2) lib/active_support/inflector/methods.rb:267:in `constantize'
activesupport (5.1.2) lib/active_support/core_ext/string/inflections.rb:66:in `constantize'
rails_sortable (0.1.1) app/controllers/sortable_controller.rb:21:in `parse_params'
rails_sortable (0.1.1) app/controllers/sortable_controller.rb:6:in `reorder'

Possible Fixes:

User Workaround (no changes to rails_sortable)

Remove auto generated hidden field with include_id: false
Add it back inside the sortable div manually eg: <%= f.hidden_field :id %>

Front end fix

Add a class selector to javascript to as to only submit sortable's
eg: replace $(this).sortable('serialize') with something like $(this).find('.a-class-selector').sortable('serialize')

Back end fix

Pre-filter params to weed out non-sortable
eg: ApplicationRecord.descendants.select{ |c| c.included_modules.include?(RailsSortable::Model) }
This is the fix I prefer

itmammoth commented 6 years ago

Thanks @jvanus

I think you need to write your .erb so that div contains input like the code below.

<div class="ui-sortable">
  <div id="Question_1" class="ui-sortable-handle">
    <input type="hidden" value="1" id="survey_questions_attributes_0_id" name="survey[questions_attributes][0][id]">
    ...
  </div>
</div>

In addition, unfortunately RailsSortable only works well with persisted model for now. If you are trying to put it into form to persist some models, please just use jquery.sortable instead, and sort them with the param's sequence.

jvanus commented 6 years ago

Yeah, thats what I did for my workaround above. All the models being sorted are persisted, they are just also editable. My proposal is really just to make the rails_sortable controller a bit more robust by removing the assumption that no extraneous data could be included in params.

itmammoth commented 6 years ago

You have a point. The rails_sortable controller is on poor implementation as you say. I will improve it some time.

itmammoth / rails_sortable