Best Practice

11155 · January 5, 2011, 9:45pm

I have an app that manages tapes. Each tape has a number (separate from the record ID in MySQL).

When a tape is added, the number field can be filled in. But when the record is subsequently edited, the number should not be editable.

Both the "new" and "edit" views include (render) the same form partial. What would be considered best practice here?

1. Logic in the partial that checks to see if the action is "new" or "edit", and changes the form appropriately

2. Two different partials.

3. Remove the partial rendering and just have the "new" and "edit" views be complete forms

I'm also interested in the "Rails philosophy" behind any recommendation. For example, I'd think that choice 1 is frowned upon, because it puts logic in a view, which should be minimized. But that choice creates the least amount of duplication, so it may make the code easier to manage long term. Which idea takes precedence?

11155 · January 5, 2011, 9:47pm

Brian Ablaza wrote in post #972628:

I have an app that manages tapes. Each tape has a number (separate from the record ID in MySQL).

When a tape is added, the number field can be filled in. But when the record is subsequently edited, the number should not be editable.

Both the "new" and "edit" views include (render) the same form partial. What would be considered best practice here?

1. Logic in the partial that checks to see if the action is "new" or "edit", and changes the form appropriately

2. Two different partials.

3. Remove the partial rendering and just have the "new" and "edit" views be complete forms

It depends how different the forms are.

Also:

4. Create a helper function to take the logic out of the view.

5. Decompose the form into more partials.

I'm also interested in the "Rails philosophy" behind any recommendation. For example, I'd think that choice 1 is frowned upon, because it puts logic in a view, which should be minimized.

I agree, though simple conditionals are usually OK in views IMHO.

But that choice creates the least amount of duplication, so it may make the code easier to manage long term. Which idea takes precedence?

There are other ways to reduce duplication, so I think it's a false choice.

Best,

Colin_Law1 · January 5, 2011, 10:21pm

Also don't forget that just making the field read only in the form will not prevent someone with malicious intent constructing a POST with a value for that attribute. Therefore, if that is a worry for you, make sure you prevent that field from being updated in the update action.

Colin

rab · January 5, 2011, 10:56pm

Rather than check the action, I'd suggest checking whether the object was new.

<% if f.object.new_record? %> <%= f.text_field :number %> <% else %> <%= f.object.number %> <% end %>

But still protect the number attribute from mass-assignment and handle the field in the create action.

-Rob

Rob Biedenharn Rob@AgileConsultingLLC.com http://AgileConsultingLLC.com/ rab@GaslightSoftware.com http://GaslightSoftware.com/

Colin_Law1 · January 6, 2011, 8:35am

I don't think I approve of doing it this way. This is putting too much intelligence into the view. The view is saying 'I know that if the object is a new record then I should display this field'. Instead I suggest one should set a flag in the controller action, then the view is just saying 'I know that if this flag is set I should show this field'. Call the flag show_id or similar and the view is just obeying orders, not making decisions.

Colin

rab · January 6, 2011, 1:00pm

Well, the half-way position would be to move just the :number attribute out of the partial and put the text_field into the 'new' view and the simple display into the 'edit' view avoiding any explicit test. My point was that there's no reason to completely abandon the idea that the fields of the model that are common to the 'new' and 'edit' actions reside in a partial.

-Rob

Rob Biedenharn Rob@AgileConsultingLLC.com http://AgileConsultingLLC.com/ rab@GaslightSoftware.com http://GaslightSoftware.com/

11155 · January 6, 2011, 1:43pm

I would add a custom param called params[:updatable] to both the create an update actions of the controller, and pass the values of true from create and false from update to the model. The model allows the field to be saved if true.

Then create a simple helper for the form logic to decide whether the field is viewable on the view.

Colin_Law1 · January 6, 2011, 1:54pm

I don't see what that accomplishes. If the field is not editable in the view then the value will not be passed in params unless a hand-crafted post is sent. If a hand-crafted post is sent then it can include the updatable flag so a malicious person can still modify the field. I believe that logic in the update action specifically not allowing the field to be updated from params is the only way.

Colin

11155 · January 6, 2011, 2:04pm

Colin Law wrote in post #972809:

I don't see what that accomplishes. If the field is not editable in the view then the value will not be passed in params unless a hand-crafted post is sent. If a hand-crafted post is sent then it can include the updatable flag so a malicious person can still modify the field. I believe that logic in the update action specifically not allowing the field to be updated from params is the only way.

Colin

You can merge the params from the controller action and the one supplied from the controller will be the one the model applies. If you are setting the param to false from the controller action before it gets sent to the model, how can it be true? It can't.

Also, you don't have to just supply a param, you can also supply a param and a conditional like you suggested on new_record. Either way works.

11155 · January 6, 2011, 2:12pm

Colin Law wrote in post #972809:

I would add a custom param called params[:updatable] to both the create an update actions of the controller, and pass the values of true from create and false from update to the model. The model allows the field to be saved if true.

I don't see what that accomplishes. If the field is not editable in the view then the value will not be passed in params unless a hand-crafted post is sent. If a hand-crafted post is sent then it can include the updatable flag so a malicious person can still modify the field. I believe that logic in the update action specifically not allowing the field to be updated from params is the only way.

...and this is why attr_protected sucks so bad. There oughta be an easy way of saying "reject these attributes, but only for certain actions". Unfortunately, Rails doesn't, and perhaps can't, work that way, so we're stuck with clumsy hash merges in the controller.

I wonder if a better way is possible. Hmm.

Colin

Best,

Colin_Law1 · January 6, 2011, 2:14pm

Colin Law wrote in post #972809:

I don't see what that accomplishes. If the field is not editable in the view then the value will not be passed in params unless a hand-crafted post is sent. If a hand-crafted post is sent then it can include the updatable flag so a malicious person can still modify the field. I believe that logic in the update action specifically not allowing the field to be updated from params is the only way.

Colin

You can merge the params from the controller action and the one supplied from the controller will be the one the model applies. If you are setting the param to false from the controller action before it gets sent to the model, how can it be true? It can't.

I think I misunderstood what you meant, you said:

I would add a custom param called params[:updatable] to both the create an update actions of the controller, and pass the values of true from create and false from update to the model. The model allows the field to be saved if true.

I thought you meant pass :updatable from the view to the create and update actions, but perhaps you meant just set them in the controller action and then pass them to the model. In that case how would you interrogate :updatable in the model, given that update_attributes will be used to do the update?

Colin

Robert_Pankowecki · January 6, 2011, 3:51pm

...and this is why attr_protected sucks so bad. There oughta be an easy way of saying "reject these attributes, but only for certain actions". Unfortunately, Rails doesn't, and perhaps can't, work that way, so we're stuck with clumsy hash merges in the controller.

I wonder if a better way is possible. Hmm.

http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html#method-i-attr_accessible

It's much more flexible and simple system in rails 3.

Robert pankowecki

11155 · January 6, 2011, 3:58pm

Robert Pankowecki wrote in post #972837:

...and this is why attr_protected sucks so bad. There oughta be an easy way of saying "reject these attributes, but only for certain actions". Unfortunately, Rails doesn't, and perhaps can't, work that way, so we're stuck with clumsy hash merges in the controller.

I wonder if a better way is possible. Hmm.

http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html#method-i-attr_accessible

#237 Dynamic attr_accessible - RailsCasts

It's much more flexible and simple system in rails 3.

They fixed this in Rails 3? Squee! That's really cool.

(I'm just starting to work with Rails 3, and I don't use attr_protected that often, so I hadn't found that out yet.)

Robert pankowecki

Best,

11155 · January 6, 2011, 11:30pm

Colin Law wrote in post #972816:

I think I misunderstood what you meant, you said:

I would add a custom param called params[:updatable] to both the create an update actions of the controller, and pass the values of true from create and false from update to the model. The model allows the field to be saved if true.

I thought you meant pass :updatable from the view to the create and update actions, but perhaps you meant just set them in the controller action and then pass them to the model. In that case how would you interrogate :updatable in the model, given that update_attributes will be used to do the update?

Colin

Yes, that is what I meant Colin.

I actually encountered something similar when going over my forum software that I was working on. I needed to decide how to allow some fields to be updated, but only in specific situations. So, I created a bitfields permissions system for authorization on controller actions and within views.

I have a permissions table with action types that have bits assigned. I can define permissions for all objects, including users, controllers, views, and even models. I'll give you a brief idea:

gist.github.com

https://gist.github.com/elricstorm/768843

example_seeds.rb

# Create permissions to populate the permissions table with.
permissions = Forumpermission.create([
    {:name => 'view', :description => 'view index', :bitfield => 1},
    {:name => 'search', :description => 'search forum', :bitfield => 2},
    {:name => 'viewtopics', :description => 'view topics', :bitfield => 4},
    {:name => 'createtopics', :description => 'create topics', :bitfield => 8},
    {:name => 'edittopic', :description => 'edit own topic', :bitfield => 16},
    {:name => 'deletetopic', :description => 'delete own topic', :bitfield => 32},
    {:name => 'editothertopics', :description => 'edit other topics', :bitfield => 64},
    {:name => 'deleteothertopics', :description => 'delete other topics', :bitfield => 128},

This file has been truncated. show original

forum_permissions.rb

# Example bitfields from Kirin Forum Software
# Author is Joel Dezenzio => Elricstorm => Alpha Blue

module ForumPermissions

  # The BitPermissions class is used to read and compare
  # bitfields between an object (user) and the currently
  # allowed permissions stored in a model (Forum Permissions).
  # It determines what actions the user object is allowed to
  # performed based upon the comparative result.

This file has been truncated. show original

But, to answer your question, I would interrogate the action with bitfield permissions.

Colin_Law1 · January 7, 2011, 9:32am

What I don't immediately see is how that links in to update_attributes to prevent particular columns being updated.

Colin

11155 · January 7, 2011, 8:59pm

Colin Law wrote in post #973053:

What I don't immediately see is how that links in to update_attributes to prevent particular columns being updated.

Colin

You can apply the same authorization on fields within models. If you have for instance 3 fields that should be updatable based on particular action responders, you can have the model update_attributes based on the currently stored value in the database, or the value being sent via params to the model based on bitfield actions accessible to the model.

In laymans terms, its like saying:

can_model(update_myfieldname, in_this_action, mymodel)

.. if the model's bitfield allows the update of the field, matching the authorization bitfield within "in_this_action", it proceeds with the params for the field value, otherwise, it just returns the currently stored value.

It's not difficult. It may seem complicated at first, but it's actually really simple to implement.

You could apply the bitfield authorization in the view (which is similar to what you proposed with new_model?, which keeps the field from being entered, but doesn't restrict a post hack. You could add another layer in the controller, if you wanted to to decide which actions are usable, and/or apply merged params, removing the duplicated field before merging, and sending to the model... and/or applying bitfield permissions in the model. While this is definitely overkill IMHO, with a system in place, you only have to add "one" line of code to the view, one line of code to the controller, and one line of code to the model.

3 lines of code.

Colin_Law1 · January 8, 2011, 8:04am

Colin Law wrote in post #973053:

What I don't immediately see is how that links in to update_attributes to prevent particular columns being updated.

Colin

You can apply the same authorization on fields within models. If you have for instance 3 fields that should be updatable based on particular action responders, you can have the model update_attributes based on the currently stored value in the database, or the value being sent via params to the model based on bitfield actions accessible to the model.

I still don't understand how you link the authorisation into update_attributes. Do you override update_attributes in the model, or are you doing it in the validations or what?

Colin

In laymans terms, its like saying:

can_model(update_myfieldname, in_this_action, mymodel)

.. if the model's bitfield allows the update of the field, matching the authorization bitfield within "in_this_action", it proceeds with the params for the field value, otherwise, it just returns the currently stored value.

It's not difficult. It may seem complicated at first, but it's actually really simple to implement.

You could apply the bitfield authorization in the view (which is similar to what you proposed with new_model?, which keeps the field from being entered, but doesn't restrict a post hack. You could add another layer in the controller, if you wanted to to decide which actions are usable, and/or apply merged params, removing the duplicated field before merging, and sending to the model... and/or applying bitfield permissions in the model. While this is definitely overkill IMHO, with a system in place, you only have to add "one" line of code to the view, one line of code to the controller, and one line of code to the model.

I get all the rest, it is the one line of code in the model that still is unclear. What is the line and where does it go?

Colin

11155 · January 9, 2011, 3:20pm

Hey Colin,

I'm in the process of putting together the bitfields permission system into a gem in case someone wants to use a different set of authorization for their app. When I get it done, I'll publish it and let you know the name and git address.

It's much easier to see how things work when you have the whole package in front of you and can see all of the code.

Thanks mate.

Colin_Law1 · January 9, 2011, 4:27pm

Thanks for that, but what I really would like to know is how you have managed, with one line of code in the model, to intercept update_attributes so that it only updates allowed attributes.

Colin