Best Practice

dubstep · January 5, 2011, 10: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?

Logic in the partial that checks to see if the action is “new” or
“edit”, and changes the form appropriately
Two different partials.
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?

zakifataya · January 5, 2011, 10:47pm

Brian A. 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?

Logic in the partial that checks to see if the action is “new” or
“edit”, and changes the form appropriately

Two different partials.

Remove the partial rendering and just have the “new” and “edit” views
be complete forms

It depends how different the forms are.

Also:

Create a helper function to take the logic out of the view.
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,

Marnen Laibow-Koser
http://www.marnen.org
[email protected]

zakifataya · January 5, 2011, 11:23pm

On 5 January 2011 21:45, Brian A. [email protected] wrote:

“edit”, and changes the form appropriately

Two different partials.

Remove the partial rendering and just have the “new” and “edit” views
be complete forms

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

zakifataya · January 5, 2011, 11:57pm

On Jan 5, 2011, at 5:21 PM, Colin L. wrote:

What would be considered best practice here?
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

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 B.
[email protected] http://AgileConsultingLLC.com/
[email protected] http://GaslightSoftware.com/

zakifataya · January 6, 2011, 2:02pm

On Jan 6, 2011, at 3:35 AM, Colin L. wrote:

When a tape is added, the number field can be filled in. But when
<%= f.object.number %>
Colin
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 B.
[email protected] http://AgileConsultingLLC.com/
[email protected] http://GaslightSoftware.com/

zakifataya · January 6, 2011, 2: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.

zakifataya · January 6, 2011, 9:37am

On 5 January 2011 22:56, Rob B. [email protected]
wrote:

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 %>

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

zakifataya · January 6, 2011, 3:04pm

Colin L. 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.

zakifataya · January 6, 2011, 2:56pm

On 6 January 2011 13:43, Alpha B. [email protected] wrote:

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.

Colin

zakifataya · January 6, 2011, 3:16pm

On 6 January 2011 14:04, Alpha B. [email protected] wrote:

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

zakifataya · January 6, 2011, 3:12pm

Colin L. wrote in post #972809:

On 6 January 2011 13:43, Alpha B. [email protected] wrote:

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,

Marnen Laibow-Koser
http://www.marnen.org
[email protected]

Sent from my iPhone

zakifataya · January 6, 2011, 4:52pm

…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

zakifataya · January 7, 2011, 12:30am

Colin L. 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.

zakifataya · January 6, 2011, 4: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,

Marnen Laibow-Koser
http://www.marnen.org
[email protected]

zakifataya · January 7, 2011, 10:38am

On 6 January 2011 23:30, Alpha B. [email protected] wrote:

…
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:

example_seeds.rb · GitHub

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

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

Colin

zakifataya · January 8, 2011, 9:06am

On 7 January 2011 20:59, Alpha B. [email protected] wrote:

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

It’s not difficult. It may seem complicated at first, but it’s actually
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

zakifataya · January 9, 2011, 4: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.

zakifataya · January 9, 2011, 5:29pm

On 9 January 2011 15:20, Alpha B. [email protected] wrote:

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 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

zakifataya · January 7, 2011, 9:59pm

Colin L. 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.