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 version timestamp to "updated_at" #26

Merged
merged 3 commits into from
Mar 3, 2017
Merged

Default version timestamp to "updated_at" #26

merged 3 commits into from
Mar 3, 2017

Conversation

akxcv
Copy link
Contributor

@akxcv akxcv commented Feb 28, 2017

Hi! A lot of Rails models have an updated_at property which is used for many things. Wouldn't it be more logical and natural to take the value from the updated_at column and use it as version's timestamp? If the record's updated_at is nil or if the table doesn't have this column, version's timestamp will be set to now().

# with updated_at column
post = Post.create! # now()
post.update!        # updated_at

# without updated_at column
post = Post.create! # now()
post.update!        # now()

Also, it's a big benefit when testing. For example, I might want to use Timecop.freeze to freeze time and then update my record (e.x. testing my version decorator). However, Timecop.freeze will not affect SQL. If version's timestamp is the same as record's updated_at, freezing time becomes trivial.

Let me know what you think about my proposal.
Also submitting a small pull request.

@palkan
Copy link
Owner

palkan commented Mar 1, 2017

Hi!
That make sense. At least while we're Rails-only.

One caveat: we should handle update_all behaviour (update_column, etc). Every update should create a new version (unless we've disabled logidze explicitly).

Thus we have to check that the updated_at value has changed (by comparing OLD and NEW) and if it hasn't – fallback to now() (or statement_timestamp(), see #27).

And it would be great to make this feature configurable (i.e. custom ts column name).

@palkan palkan self-requested a review March 1, 2017 09:29
@akxcv
Copy link
Contributor Author

akxcv commented Mar 1, 2017

Thanks for the review. Going to update the PR soon.
Looks like #27 is almost as simple as find&replace. We might close it with this PR as well.

@palkan
Copy link
Owner

palkan commented Mar 1, 2017

Looks like #27 is almost as simple as find&replace. We might close it with this PR as well.

It would be great)

@@ -1,3 +1,4 @@
# rubocop:disable Lint/HandleExceptions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/UnneededDisable: Unnecessary disabling of Lint/HandleExceptions.

@akxcv
Copy link
Contributor Author

akxcv commented Mar 2, 2017

Weird, Hound and Rubocop do not agree on code style.

@palkan
Copy link
Owner

palkan commented Mar 2, 2017

Looks good! 👍

Let's squash the last three commits into one and merge it.

describe "Logidze timestamps", :db do
include_context "cleanup migrations"

def self.setup_models(timestamp_column:)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's convert this method into a shared context with a parameter to be more RSpec way.

…stamp()

Fix code style according to Hound and Rubocop

Make Hound agree with Rubocop

refinements
@akxcv
Copy link
Contributor Author

akxcv commented Mar 3, 2017

Looks like github doesn't notify about a PR update when you squash your commits. Leaving this comment to force a notification. :)

@palkan
Copy link
Owner

palkan commented Mar 3, 2017

One more thing I forgot: please, update the changelog.

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.

3 participants