#5185 unchanged attribute is changed [ActiveModel] - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5185 ✓resolved

unchanged attribute is changed [ActiveModel]

Reported by eagle.anton (at gmail) | July 23rd, 2010 @ 01:37 PM | in 3.x

ActiveModel::Dirty has the example:

# Assigning the same value leaves the attribute unchanged:
#   person.name = 'Bill'
#   person.name_changed?  # => false
#   person.name_change    # => nil

It isn't working.

Test in attachment failes.

dirty.diff 837 Bytes

Comments and changes to this ticket

You flagged this item as spam.
eagle.anton (at gmail) July 23rd, 2010 @ 02:25 PM
- Tag set to “rails3 active_model dirty”
You flagged this item as spam.
lakshmanan July 24th, 2010 @ 01:32 PM
works for me ! It(Ticket) seems invalid ..

I tried this in console as well.
You flagged this item as spam.
lakshmanan July 24th, 2010 @ 02:53 PM
- Tag changed from “rails3 active_model dirty” to “rails3 active_model dirty, invalid”
Rohit Arondekar July 25th, 2010 @ 02:39 AM
- State changed from “new” to “open”
- Assigned user set to “josh”
- Tag changed from “rails3 active_model dirty, invalid” to “rails 3, activemodel, dirty”
- Importance changed from “” to “Low”
Confirmed that the provided test fails on Rails master.

Can you try writing a patch to fix this?
You flagged this item as spam.
Tore Darell July 25th, 2010 @ 12:53 PM
Maybe I'm missing something, but I don't think the test from the diff should pass, using the example implementation from Dirty. The attribute_will_change! method says to Dirty that it will change, so it adds the attribute name and value to @changed_attributes@, which is used by attribute_changed? to determine if it's been changed or not, using changed_attributes.include?(attr).

It seems to me it's up to the implementer to decide when an attribute changes or not, and in the example class a change is recorded every time name= is called. Maybe the examples were copied from when this was part of ActiveRecord, where the test would pass AFAIK.
Rohit Arondekar July 25th, 2010 @ 01:07 PM
I think Tore is right. By calling name_will_change! in Person#name=, it's already decided that the attribute has changed.

If this is the desired behavior, which I think is a good idea, then the API docs need to be fixed.
Rohit Arondekar July 25th, 2010 @ 01:56 PM
Calling attr_will_change! accordingly is fine but I don't think there is a method that marks a attribute or the entire object as clean.

So how do I tell AMo that the object is now clean in my save method? That is, all the pending changes have been applied so that model.changed? #=> false. I'm guessing it can be done by changing the @changed_attributes map manually.

Would be useful to do will_be_clean! and maybe even attr_will_be_clean!.
You flagged this item as spam.
Tore Darell July 25th, 2010 @ 02:16 PM
It looks like ARec does this.. From http://github.com/rails/rails/blob/master/activerecord/lib/active_record/attribute_methods/dirty.rb#L23:
```
     def save(*) #:nodoc:
        if status = super
          @previously_changed = changes
          @changed_attributes.clear
        end
        status
      end
```
Rohit Arondekar July 26th, 2010 @ 07:59 AM
The API docs: http://edgeapi.rubyonrails.org/classes/ActiveModel/Dirty.html are pretty misleading. The given examples don't work on the example class given. And they won't work unless the user adds more code. 'Save' and 'Assigning the same value leaves the attribute unchanged' don't work for ex.

Should the example Person class be beefed up to support the examples? Or should the non-working examples be removed?

Also how about providing convenience methods like clear_changes! for @changed_attributes.clear and archive_changes! for @previously_changed = changes? Maybe even a archive_and_clear_changes! for doing both in one shot.
You flagged this item as spam.
Tore Darell July 26th, 2010 @ 03:11 PM
Attaching a patch which fixes the docs and adds a few more tests.
- 0001-remove-or-fix-non-working-examples-add-a-few-more-te.patch 3.4 KB
You flagged this item as spam.
Tore Darell July 26th, 2010 @ 03:48 PM
Left out the original issue from the patch, but this should include it..
- 0001-remove-or-fix-non-working-examples-and-add-a-few-tes.patch 4.3 KB
Rohit Arondekar July 26th, 2010 @ 04:19 PM
- Assigned user changed from “josh” to “José Valim”
- Milestone cleared.
Thanks Tore! :)

Just one very small nitpick though — The test "setting name will result in change" could be named as "setting attribute will result in change".
José Valim August 2nd, 2010 @ 04:05 PM
Patch no longer applies, could you please rebase? Thanks!
José Valim August 2nd, 2010 @ 04:05 PM
- Milestone set to “3.x”
You flagged this item as spam.
Tore Darell August 2nd, 2010 @ 08:20 PM
Changed.
- 0001-remove-or-fix-non-working-examples-and-add-a-few-tes.patch 4.4 KB
Rohit Arondekar August 3rd, 2010 @ 08:12 AM
Jose, any reason why this is not going into 3.0? It's not changing the API, only fixing the docs and adding some more tests.
Repository August 3rd, 2010 @ 09:53 AM
- State changed from “open” to “resolved”
(from [2c8a4a53a8c38a43a62342b9d46014242e781d18]) Remove or fix non-working examples and add a few tests to Dirty [#5185 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/2c8a4a53a8c38a43a62342b9d46014...
Ryan Bigg November 8th, 2010 @ 01:51 AM
- Tag cleared.
Automatic cleanup of spam.