CODEWITHSUNDEEP
htmlruby-on-railsruby-on-rails-3forms
Ben
Ben

Reputation: 472

Why is directly manipulating the Rails form params hash considered code smell?

I have a Rails form with a parent model and nested attributes for potentially multiple children of another model.

The child model has an attribute which is manipulated in logic as an array, but is serialized to a YAML string using the Rails built-in serialize method.

Within the form, I display each individual member of the array so that the user can selectively delete members.

The problem happens when the user destroys all members. The form will not pass any value for the param to the Rails controller and when the UPDATE action is called, it ignores the attribute since there is no key for it in the forms params hash. This is of course a known problem with things like checkboxes, so Rails automatically puts 2 checkbox HTML elements for each checkbox, one hidden that only processes if the checkbox is checked off.

I'm not dealing with checkboxes here but rather hidden input text fields.

The solution I've implemented is to manipulate the params hash directly in the UPDATE action of the controller, like this:

params[:series][:time_slots_attributes].each { |k,v| v[:exdates] ||= [] }

Is this considered code smell?

Should I instead add an extra hidden field that is disabled and only gets enabled when the user removes the last member? This solution works as well, but it seems clunky to me.

Upvotes: 0

Views: 740

Answers (2)

Ben
Ben

Reputation: 472

This is far from an exhaustive answer...but after thinking about this problem, one issue I can see is that if future forms are built that leverage the same UPDATE action, unexpected behavior will occur, which violates the principle of least surprise. If at a later time a second form is built which does not expect to change values for the exdates attribute (since it does not pass them), the UPDATE action will write an empty array into the attribute anyway.

I've decided to solve this issue by adding a single hidden form field with a true boolean value and later to check for this value before setting all time slot exdates to an empty array. This way, if a future developer creates a new form that leverages the UPDATE action of the series controller, they won't get the unexpected behavior of their exdates being set to empty arrays. If they want to process exdates in their form, they need to have the same hidden form field with a true value. This seemed like a simpler solution then adding a class and table for exdates, migration, and the AR associations and adding another layer of nested attributes so that I'd have not only a parent and children attributes, put a parent, children and grandchildren. This solution is a bit like the Rails hack for dealing with checkboxes with a second hidden checkbox field in the form.

Upvotes: 0

Gareth
Gareth

Reputation: 138110

This is dealt with in the NestedAttributes module by allowing a "_destroy" parameter to trigger a destroy call for that particular nested attribute:

http://apidock.com/rails/ActiveRecord/NestedAttributes/ClassMethods/accepts_nested_attributes_for.

If you're not using nested attributes (which you probably should be, it's pretty neat in a lot of situations) then yes, you'll have to handroll something yourself, by working out which values should have been present and doing something special with those.

Upvotes: 1

Related Questions