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

clear rolled back versions from associated model #440

Merged
merged 4 commits into from
Nov 11, 2014
Merged

clear rolled back versions from associated model #440

merged 4 commits into from
Nov 11, 2014

Conversation

moserrya
Copy link
Contributor

@moserrya moserrya commented Nov 6, 2014

I propose this change to fix an issue where, under the right circumstances, version records are created for changes that never took place. Let's say I have two models, Item and Location, and attempt to save changes in a transaction:

item.transaction do
  item.update_attributes!(sku: 'AYH123HJLF')
  Location.first.update_attributes!(code: '999-999')
end

The Item model here has_paper_trail. A version is created for the item's update_attributes! call is created and then rolled back, as expected. However, if we rescue from the error that happens when we run Location.first.update_attributes! and then call item.save, the rolled back version created when we tried to update the sku in a transaction is also saved - even though the sku on the item is not changed.

An alternative approach - could try setting autosave: false in has_paper_trail

@batter
Copy link
Collaborator

batter commented Nov 10, 2014

I think this makes sense.. I'm guessing the concern here is a failure on the first update, then a second update going through and a version being persisted for a state change that never happened? Any chance you can add some tests?

@moserrya
Copy link
Contributor Author

This is exactly the concern. I will add tests.

@batter batter added this to the 3.1.0 milestone Nov 10, 2014
@batter batter merged commit 44e4332 into paper-trail-gem:master Nov 11, 2014
@batter
Copy link
Collaborator

batter commented Nov 11, 2014

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants