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

Default order by timestamp to ensure early versions are cleaned #701

Merged
merged 1 commit into from Jan 20, 2016

Conversation

owenr
Copy link
Contributor

@owenr owenr commented Jan 19, 2016

Addresses #699: 'cleaner' intermittently fails to keep the latest version because the query assumes but does not enforce chronological order. Postgresql exposes the assumption by failing the tests but not 100% of the time.

I had considered ordering by id instead of timestamp but was unsure about the reliability of that approach.

@bronson
Copy link

bronson commented Jan 19, 2016

Very nice. Personally I find ordering by ID more reliable since timestamps can end up equal.

@owenr
Copy link
Contributor Author

owenr commented Jan 19, 2016

How about order(PaperTrail.timestamp_field, :id) ?

@bronson
Copy link

bronson commented Jan 19, 2016

Sure, that fixes the equality problem. Does it have a benefit over sorting by id only?

@owenr
Copy link
Contributor Author

owenr commented Jan 19, 2016

If someone changed the assignment of IDs in their database I could see it being a problem, but having checked it looks like the table will always be created with monotonically-increasing IDs. @jaredbeck, any preference?

@jaredbeck
Copy link
Member

The documentation for the :keeping option says:

  • :keeping - An integer indicating the number of versions to be kept for
    each item per date. Defaults to 1.

Can you add something about how the one that's kept is the most recent?

For the sort order, I'd suggest order(PaperTrail.timestamp_field, :id) because it just makes sense to use timestamps for a chronological ordering. Scott makes a good point about timestamp equality, and adding the id as a secondary sort should give us deterministic results in that case.

Finally, please add a line to the changelog, under 5.0.0 -> Fixed. Thanks!

@owenr
Copy link
Contributor Author

owenr commented Jan 20, 2016

Done.

jaredbeck added a commit that referenced this pull request Jan 20, 2016
Default order by timestamp to ensure early versions are cleaned
@jaredbeck jaredbeck merged commit cfcd11d into paper-trail-gem:master Jan 20, 2016
@jaredbeck
Copy link
Member

Thanks!

@batter
Copy link
Collaborator

batter commented Jan 20, 2016

I believe we need should update this to use PaperTrail::Version.timestamp_sort_order. Apologies for the delayed response, just saw this, I'm happy to work that in tomorrow when I get a chance.

@batter
Copy link
Collaborator

batter commented Jan 20, 2016

Made the aforementioned update: c200721

@jaredbeck
Copy link
Member

Thanks Ben. Good catch.

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