I''ve been playing around with Dirty (change tracking) recently, and
have
a question about the field_changed? method:
def field_changed?(attr, old, value)
if column = column_for_attribute(attr)
if column.type == :integer && column.null && (old.nil? ||
old == 0)
# For nullable integer columns, NULL gets stored in database for
blank (i.e. '''') values.
# Hence we don''t record it as a change if the value changes
from
nil to ''''.
# If an old value of 0 is set to '''' we want this to
get changed
to nil as otherwise it''ll
# be typecast back to 0 (''''.to_i => 0)
value = nil if value.blank?
else
value = column.type_cast(value)
end
end
old != value
end
I understand the reasoning behind the if...else statement, but what
about the situation where you''re changing a nullable integer column
from
0 to ''0''? In this case it will go into the if block, leave
''value''
unchanged, and end up returning true - this seems incorrect to me!
If the value.blank? test was moved to the end of the long if statement,
the else block would be processed instead, so the value typecasted to 0,
and return false, which is what I would expect (and (obviously) is what
happens with non-nullable integer columns).
Am I missing something, or should I go ahead and write the patch?
Thanks all.
--~--~---------~--~----~------------~-------~--~----~
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 3 Dec 2008, at 12:08, Ben Symonds wrote:> > I''ve been playing around with Dirty (change tracking) recently, and > have > a question about the field_changed? method:I''ve been looking at this with ben and it does seem a bit messed up With a table like so: create_table :products do |t| t.integer :quantity, :null => false t.integer :product_id, :null => true t.timestamps end p = Product.create :quantity => 0, :product_id => 0 => #<Product id: 2, quantity: 0, product_id: 0, created_at: "2008-12-04 10:34:04", updated_at: "2008-12-04 10:34:04"> >> p.changed? #=> false >> p.quantity = ''0'' >> p.changed? #=> false >> p.product_id = ''0'' >> p.changed? #=> true It''s not going to happen that often since this only happens when the existing value is 0, and only affects integer columns that can be null. Still, doesn''t seem right to me. WHat ben has suggested certainly seems to do the right thing Fred --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
Well since noone objected, I went ahead and created a patch. See: http://rails.lighthouseapp.com/projects/8994/tickets/1530 Any comments appreciated! Frederick Cheung wrote:> On 3 Dec 2008, at 12:08, Ben Symonds wrote: > > >> I''ve been playing around with Dirty (change tracking) recently, and >> have >> a question about the field_changed? method: >> > > I''ve been looking at this with ben and it does seem a bit messed up > > With a table like so: > > create_table :products do |t| > t.integer :quantity, :null => false > t.integer :product_id, :null => true > t.timestamps > end > > p = Product.create :quantity => 0, :product_id => 0 > => #<Product id: 2, quantity: 0, product_id: 0, created_at: > "2008-12-04 10:34:04", updated_at: "2008-12-04 10:34:04"> > >> p.changed? #=> false > >> p.quantity = ''0'' > >> p.changed? #=> false > >> p.product_id = ''0'' > >> p.changed? #=> true > > It''s not going to happen that often since this only happens when the > existing value is 0, and only affects integer columns that can be > null. Still, doesn''t seem right to me. WHat ben has suggested > certainly seems to do the right thing > > Fred > > > >--~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---