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

Integrate versioning into AR touch method #1063

Merged
merged 3 commits into from Mar 16, 2018

Conversation

Projects
None yet
4 participants
@westonganger
Copy link
Contributor

commented Mar 13, 2018

Related to Issue 1061

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

Something to consider with this PR is the following lines:

https://github.com/westonganger/paper_trail/blob/7dafd009716bfcef760e26bfa45f6274a2caf0bb/lib/paper_trail/model_config.rb#L203-L208

Currently it will attach to the touch event always. Maybe this should be added as one of the available :on events and it included by default or must be specified?

@jaredbeck
Copy link
Member

left a comment

Looking good. Should we deprecate touch_with_version?

@@ -196,6 +204,8 @@ def setup_callbacks_from_options(options_on = [])
options_on.each do |event|
public_send("on_#{event}")
end

public_send("on_touch")

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

I think you can just call on_touch here, right? The public_send is not necessary.

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

Currently it will attach to the touch event always. Maybe this should be added as one of the available :on events and it included by default or must be specified?

Good thinking. I think the callback should be installed by default, but configurable via the :on option.

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

This will need a changelog entry and probably some significant changes to documentation in comments and in the readme.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

I guess touch_with_version could be deprecated. Should that be a part of a separate PR? or would you want to do this yourself?

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

Okay the following items have been added to this PR:

  • Add touch to list of :on events (by default it is included)
  • Deprecate touch_with_version
  • Updated changelog
  • Updated readme documentation to use touch instead of touch_with_version
def touch_with_version(name = nil)
::ActiveSupport::Deprecation.warn(DPR_TOUCH_WITH_VERSION, caller(1))

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

Nice deprecation implementation

@@ -563,17 +570,16 @@ def attribute_changed_in_latest_version?(attr_name)
# Rails 5.1 changed the API of `ActiveRecord::Dirty`. See
# https://github.com/airblade/paper_trail/pull/899
#
# Event can be any of the three (create, update, destroy).
# Event can be any of the following (create, update, destroy, touch).

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

"touch" is not an event. There are still only three events: create, update, and destroy. touch will cause the insertion of an "update" event. See record_update.

I think this will be confusing to many people, and I'm wondering how we can document it better.

This comment has been minimized.

Copy link
@westonganger

westonganger Mar 14, 2018

Author Contributor

Well should we make touch an event? I would simply add another optional argument to record_update(force:, in_after_callback:, event: "update") or add a record_touch method.

Otherwise I can simply add documentation in the readme next to the touch documentation stating that the event will be recorded as update

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

should we make touch an event?

I don't know.

Reasons against: A touch is simply an update. It updates timestamps. If we had a touch event, the lifecycle would be harder to understand. Instead of eg. create-update-update-destroy, it's eg. create-update-touch-touch-update-touch-destroy. The "middle part" of the lifecycle becomes heterogeneous. This heterogeneity would be a major breaking change.

Reasons for: Maintains a one-to-one correspondence between callbacks and events, which makes callbacks easier to understand I guess. It's not a big advantage.

So I guess I'm leaning against making touch an event.

This comment has been minimized.

Copy link
@westonganger

westonganger Mar 14, 2018

Author Contributor

Okay then in that case, I simply updated the readme stating the event will be saved as update

@@ -5,9 +5,10 @@

module On
RSpec.describe Update, type: :model, versioning: true do
let(:record) { described_class.create(name: "Alice") }

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

Please do not use let. I believe it makes tests harder to understand, especially when it's too far away, lexically, from the test.

count = record.versions.count
expect(count).to eq(count)
end
end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

Good test. Thanks for finding and using the existing models.

count = widget.versions.count
widget.paper_trail.without_versioning do
expect(count).to eq(count)
end

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Mar 14, 2018

Member

I don't understand the implementation of this test. It's like saying expect(7).to eq(7), right? The local variable count is unchanging, right?

@jaredbeck jaredbeck merged commit 02b6de2 into paper-trail-gem:master Mar 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

Thanks, Weston.

@gduprey

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

Is there a way to globally disable the tracking of the touch event? Or more generically, set the default /create/update/destroy/touch global 'on:' defaults ?

As implemented, it breaks when using the delay_touching gem (specific case is when a delayed touch is for a record that was marked for destruction, the attempt to create the version association in record_update() in record_trail.rb throws an exception because the @record no longer is persisted (as it was deleted before the touch events are fired - touch having been delayed as the function of the delay_touching gem).

Given this was not the default behavior before and the alternative is updating potentially hundreds of models (we have a pretty large app) to disable on: :touch, I'd much rather have a PaperTrail.config.xxxx setting once.

We upgraded from 7.x to 10.x and had this break (it worked before, likely because it was before touch versioning was done)

@ghiculescu

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

@gduprey #1166 adds this

@ghiculescu

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

.... of course it does, you wrote it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.