Best Practice

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?

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,

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 Biedenharn
Rob@AgileConsultingLLC.com http://AgileConsultingLLC.com/
rab@GaslightSoftware.com http://GaslightSoftware.com/

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

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/

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.

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

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.

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

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

http://railscasts.com/episodes/237-dynamic-attr-accessible

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

Robert pankowecki

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

http://railscasts.com/episodes/237-dynamic-attr-accessible

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,

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:

https://gist.github.com/768843

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

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. :slight_smile:

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

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.

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