Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added rollback method to ActiveModel::Dirty #14861

Merged
merged 1 commit into from Jun 30, 2014

Conversation

@igor04
Copy link
Contributor

igor04 commented Apr 24, 2014

Maybe I have missed something but looks like rollback changes should be presented here

@estsauver
Copy link
Contributor

estsauver commented Apr 27, 2014

Hey @igor04

What's the difference between this and reset_changes? Am I missing something? It looks to me like they both do the same things?

~Earl

@igor04
Copy link
Contributor Author

igor04 commented Apr 27, 2014

Hi @estsauver

Not exactly, reset_changes - when you want to reset the changes information not rollback value, just delete information about changes. After reset_changes attribute will have new value but it will look like attribute has not been changed (because data about changes is deleted).

@timraymond
Copy link
Contributor

timraymond commented Jun 21, 2014

@igor04 So reset_changes will only make it appear as though nothing has changed, even though any changes that you've made still stick around? If that's the case, I'd say this patch is a win.

@sgrif
sgrif reviewed Jun 21, 2014
View changes
activemodel/lib/active_model/dirty.rb Outdated
@@ -176,6 +188,11 @@ def reset_changes
@changed_attributes = ActiveSupport::HashWithIndifferentAccess.new
end

# Restore all previous data
def rollback_changes
changed.each { |attr| reset_attribute! attr }

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 21, 2014

Contributor

This should call changed_attributes.each_key, changed gets overridden by ActiveRecord::AttributeMethods::Dirty to include in-place mutations.

@sgrif
sgrif reviewed Jun 21, 2014
View changes
activemodel/lib/active_model/dirty.rb Outdated
@@ -176,6 +188,11 @@ def reset_changes
@changed_attributes = ActiveSupport::HashWithIndifferentAccess.new
end

# Restore all previous data
def rollback_changes

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 21, 2014

Contributor

This method is private, you need to add # :api: public if you want it to be documented.

This comment has been minimized.

Copy link
@igor04

igor04 Jun 22, 2014

Author Contributor

@sgrif, what do you mean "you need to add # :api: public if you want it to be documented"? I just wrote this method in the same style as reset_changes or changes_applied.

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 22, 2014

Contributor

Sorry, it should have the # :doc: flag added. Private methods are not documented by default, this flag will make sure it is documented. You should add it on the same line as the method definition like so:

def rollback_changes # :doc:

http://rdoc.sourceforge.net/doc/

This comment has been minimized.

Copy link
@igor04

igor04 Jun 22, 2014

Author Contributor

thanks @sgrif for explanation, I will add this, but why methods like reset_changes don't have it?)

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 22, 2014

Contributor

Probably an oversight, I'll add it to them.

@sgrif
sgrif reviewed Jun 21, 2014
View changes
activemodel/test/cases/dirty_test.rb Outdated
assert_equal 'Dmitry', @model.name
assert_equal 'Red', @model.color

assert_equal ActiveSupport::HashWithIndifferentAccess.new, @model.changed_attributes

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 21, 2014

Contributor

This feels like it tests implementation details, more than anything useful. How about this, instead?

assert_not @model.changed?
@sgrif
sgrif reviewed Jun 21, 2014
View changes
activemodel/test/cases/dirty_test.rb Outdated
@model.name = 'Bob'
@model.color = 'White'

assert_equal ['Dmitry', 'Bob'], @model.changes['name']

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 21, 2014

Contributor

This seems irrelevant to the method being tested. What do you think about instead asserting that the model was changed, or that the individual attributes were changed?

@rafaelfranca rafaelfranca merged commit 552d4d8 into rails:master Jun 30, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Jun 30, 2014
Added rollback method to ActiveModel::Dirty
rafaelfranca added a commit that referenced this pull request Jun 30, 2014
@igor04 igor04 deleted the igor04:dirty-rollback branch Jul 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.