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

Transaction support for restoring associations #345

Closed
wants to merge 8 commits into from

Conversation

lyfeyaj
Copy link
Contributor

@lyfeyaj lyfeyaj commented Mar 16, 2014

This pull request is mostly a re-implementation of the work done by @tderks and @cambridgemike with some bug fix. I didn't add tests for this pull request. Anyway, I think this solution for has-many association is ready and are already in our company's production mode. And the reason I open this pull request here is want to welcome anybody to help to test it to find if there are still some bugs and if possible add tests for it.

@batter batter added this to the 3.1.0 milestone Mar 17, 2014
@batter batter self-assigned this Mar 17, 2014
@dmitry
Copy link

dmitry commented May 24, 2014

Hope here will be a full test coverage.

@lyfeyaj if you have a time for pairing, we can try to write some code together to get this done and push to the master.

@NullVoxPopuli
Copy link
Contributor

Is this the preferred method for taking care of has_manies? in my code, I am storing the other papertrail record ids in a metadata field. What's the advantage of using another table? Does that hurt speed at all? I bet it would be easier to migrate though.

@NullVoxPopuli
Copy link
Contributor

I submitted a PR for fixing the merge conflicts: lyfeyaj#2

@NullVoxPopuli
Copy link
Contributor

@lyfeyaj why is the transaction_id being stored on the module? (PaperTrail.transaction_id)
That sounds like a recipe for disaster in a production environment running multiple instances of the code, or for background workers.

@batter
Copy link
Collaborator

batter commented Dec 11, 2014

Thanks so much for this PR @lyfeyaj! Closed in favor of #439

@batter batter closed this Dec 11, 2014
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.

None yet

4 participants