Rails core - Feb 2008 - ActiveRecord::Base#toggle! - bad implementation or documentation

ActiveRecord::Base#toggle! is used to toggle an attribute true or  
false, and immediatly save it. It is implemented to call the toggle  
method, and then call update_attribute to save to the database.

The problem I see with this is that there is no validation, when  
toggling a boolean. In my opinion this is wrong, and there should be  
validation. I''ve heard people say this behaviour was correct, and  
that''s how they wanted it, in which case the documentation is  
incomplete and can lead to unexpected results.

def toggle!(attribute) toggle(attribute).update_attribute(attribute,  
self[attribute]) end
You tell me, a patch to make toggle! validate or a documentation patch  
to say toggle! doesn''t validate when saving.

I have a ticket open at http://dev.rubyonrails.org/ticket/11098 about  
this.

Regards,

Jan

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I''d personally love to see #toggle! removed from ActiveRecord
entirely.  I can''t think of a single use case for #toggle! that
isn''t
bug-prone, or that wouldn''t be better implemented by setting flags to
explicit values.

Tammer

On Feb 13, 5:18 am, Jan De Poorter <j...@openminds.be>
wrote:
> ActiveRecord::Base#toggle! is used to toggle an attribute true or  
> false, and immediatly save it. It is implemented to call the toggle  
> method, and then call update_attribute to save to the database.
>
> The problem I see with this is that there is no validation, when  
> toggling a boolean. In my opinion this is wrong, and there should be  
> validation. I''ve heard people say this behaviour was correct, and
 
> that''s how they wanted it, in which case the documentation is  
> incomplete and can lead to unexpected results.
>
> def toggle!(attribute) toggle(attribute).update_attribute(attribute,  
> self[attribute]) end
> You tell me, a patch to make toggle! validate or a documentation patch  
> to say toggle! doesn''t validate when saving.
>
> I have a ticket open athttp://dev.rubyonrails.org/ticket/11098about  
> this.
>
> Regards,
>
> Jan
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Thu, Feb 14, 2008 at 4:33 AM, Tammer Saleh <tsaleh@gmail.com>
wrote:
>
> I''d personally love to see #toggle! removed from ActiveRecord
> entirely.  I can''t think of a single use case for #toggle! that
isn''t
> bug-prone, or that wouldn''t be better implemented by setting flags
to
> explicit values.

I heartily second this.  I haven''t used it since I ran into a situation
where I was mis-guessing the initial state of a boolean.

::Jack Danger

> On Feb 13, 5:18 am, Jan De Poorter <j...@openminds.be> wrote:
> > ActiveRecord::Base#toggle! is used to toggle an attribute true or
> > false, and immediatly save it. It is implemented to call the toggle
> > method, and then call update_attribute to save to the database.
> >
> > The problem I see with this is that there is no validation, when
> > toggling a boolean. In my opinion this is wrong, and there should be
> > validation. I''ve heard people say this behaviour was correct,
and
> > that''s how they wanted it, in which case the documentation is
> > incomplete and can lead to unexpected results.
> >
> > def toggle!(attribute) toggle(attribute).update_attribute(attribute,
> > self[attribute]) end
> > You tell me, a patch to make toggle! validate or a documentation patch
> > to say toggle! doesn''t validate when saving.
> >
> > I have a ticket open athttp://dev.rubyonrails.org/ticket/11098about
> > this.
> >
> > Regards,
> >
> > Jan
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Sure, I can: boolean fields that can be toggled by administrators.  I
use a fair number of these in a project I''m doing.  #toggle! is
exactly what I would have implemented had Rails not had the feature to
begin with.

James

On Feb 14, 7:33 am, Tammer Saleh <tsa...@gmail.com>
wrote:
> I''d personally love to see #toggle! removed from ActiveRecord
> entirely.  I can''t think of a single use case for #toggle! that
isn''t
> bug-prone, or that wouldn''t be better implemented by setting flags
to
> explicit values.
>
> Tammer
>
> On Feb 13, 5:18 am, Jan De Poorter <j...@openminds.be> wrote:
>
> > ActiveRecord::Base#toggle! is used to toggle an attribute true or  
> > false, and immediatly save it. It is implemented to call the toggle  
> > method, and then call update_attribute to save to the database.
>
> > The problem I see with this is that there is no validation, when  
> > toggling a boolean. In my opinion this is wrong, and there should be  
> > validation. I''ve heard people say this behaviour was correct,
and  
> > that''s how they wanted it, in which case the documentation is
 
> > incomplete and can lead to unexpected results.
>
> > def toggle!(attribute) toggle(attribute).update_attribute(attribute,  
> > self[attribute]) end
> > You tell me, a patch to make toggle! validate or a documentation patch
 
> > to say toggle! doesn''t validate when saving.
>
> > I have a ticket open athttp://dev.rubyonrails.org/ticket/11098about 
> > this.
>
> > Regards,
>
> > Jan
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I''d call that a bad practice, and I think it''s commonly used
only
because of the existence of AR''s #toggle! method.

The "toggling" an admin flag via a web form is really "setting
the
flag to true or false, depending on what the form thinks it currently
is".  The problem with using #toggle! here is that when two
administrators both load the form and then both click the "publish"
button (or whatnot), the record will end up being unpublished.

The correct solution is very easy in a restful application, where the
"publish" link should put params[:record][:published] = true to the
update action.  But even in non-restful applications, having the link
post to #publish and #unpublish, depending on the state of the record
when the page is loaded is much more explicit, and less error prone.

This, and the fact that #toggle! ignores validates (the original point
of the thread), makes #toggle! stick out as a very non-rails way of
doing things.

The existence of #toggle! encourages its use.  It should be removed,
if only to guide new rails programmers toward best practices.

Tammer

On Feb 15, 10:23 am, "James H." <james.herd...@gmail.com>
wrote:
> Sure, I can: boolean fields that can be toggled by administrators.  I
> use a fair number of these in a project I''m doing.  #toggle! is
> exactly what I would have implemented had Rails not had the feature to
> begin with.
>
> James
>
> On Feb 14, 7:33 am, Tammer Saleh <tsa...@gmail.com> wrote:
>
> > I''d personally love to see #toggle! removed from ActiveRecord
> > entirely.  I can''t think of a single use case for #toggle!
that isn''t
> > bug-prone, or that wouldn''t be better implemented by setting
flags to
> > explicit values.
>
> > Tammer
>
> > On Feb 13, 5:18 am, Jan De Poorter <j...@openminds.be> wrote:
>
> > > ActiveRecord::Base#toggle! is used to toggle an attribute true or
 
> > > false, and immediatly save it. It is implemented to call the
toggle  
> > > method, and then call update_attribute to save to the database.
>
> > > The problem I see with this is that there is no validation, when
 
> > > toggling a boolean. In my opinion this is wrong, and there should
be  
> > > validation. I''ve heard people say this behaviour was
correct, and  
> > > that''s how they wanted it, in which case the
documentation is  
> > > incomplete and can lead to unexpected results.
>
> > > def toggle!(attribute)
toggle(attribute).update_attribute(attribute,  
> > > self[attribute]) end
> > > You tell me, a patch to make toggle! validate or a documentation
patch  
> > > to say toggle! doesn''t validate when saving.
>
> > > I have a ticket open
athttp://dev.rubyonrails.org/ticket/11098about 
> > > this.
>
> > > Regards,
>
> > > Jan
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 16.2.2008, at 20.43, Tammer Saleh wrote:
> The correct solution is very easy in a restful application, where the
> "publish" link should put params[:record][:published] = true to
the
> update action.  But even in non-restful applications, having the link
> post to #publish and #unpublish, depending on the state of the record
> when the page is loaded is much more explicit, and less error prone.
Of course, the correct solution would be not to use a link (or GET) to  
do such an action in the first place. Other than, agreed.

//jarkko

--
Jarkko Laine
http://jlaine.net
http://dotherightthing.com
http://www.railsecommerce.com
http://odesign.fi

Also, if for whatever reason the request was retryed (by a proxy
perhaps) even a single user''s actions could result in an unknown final
toggle state.

+1 for removal

On Feb 16, 2008 1:05 PM, Jarkko Laine <jarkko@jlaine.net>
wrote:
> On 16.2.2008, at 20.43, Tammer Saleh wrote:
> > The correct solution is very easy in a restful application, where the
> > "publish" link should put params[:record][:published] = true
to the
> > update action.  But even in non-restful applications, having the link
> > post to #publish and #unpublish, depending on the state of the record
> > when the page is loaded is much more explicit, and less error prone.
>
> Of course, the correct solution would be not to use a link (or GET) to
> do such an action in the first place. Other than, agreed.
>
> //jarkko
>
> --
> Jarkko Laine
> http://jlaine.net
> http://dotherightthing.com
> http://www.railsecommerce.com
> http://odesign.fi
>
>
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> Also, if for whatever reason the request was retryed (by a proxy
> perhaps) even a single user''s actions could result in an unknown
final
> toggle state.
>
> +1 for removal
Proxies don''t retry non-idempotent requests.  If they did we''d
have a
whole pile of other issues.

Base#toggle works just fine for its intended use case, toggling a
boolean value.   There are dozens of cases where this is perfectly
valid and the concurrency issues raised aren''t the end of the world.
tadalist''s todo lists for example.

Some patches to improve the documentation would be greatly
appreciated, but the feature''s definitely not
''useless'' :)

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---