Track changes on destroy

[seanlinsley]

Part 2 of #1099, followup to #1122.

Rails' changes method doesn't return any attributes on destroy, so this PR generates them by hand (though create and update still rely on Rails). In order to still apply serialization to the changes, some of the internal API has been refactored.

• get the tests passing
• decide on the versioning / deprecation plan
• document the upgrade plan for each path. explain why it's a good idea to use the new behavior
• write SQL & Ruby migrations as examples for those who want to populate object_changes on historical data

[jaredbeck]

Regarding the config option,

PaperTrail.config.record_changes_on_destroy = true

You were planning on doing a normal deprecation cycle, which would normally be the right call,

1. in a minor version: deprecate save_changes, and introduce a setting to enable object_changes on destroy
2. in the next major version: remove save_changes, and track changes on destroy by default

but the next release will be a major version, so I'd rather just start always tracking changes on destroy, and not bother with the intermediate step of having a config option.

I don't understand the rationale for removing save_changes though.

I think #507 which introduced save_changes was misguided: they were trying to reduce the amount of space used by a large text column, but didn't seem to realize that object would duplicate that column on every save.

Right, but people might want to keep save_changes so they can have it true for some models and false for others.

[seanlinsley]

When trying to save disk space, save_changes is the wrong option b/c even with it enabled, every attribute is duplicated in the object column every time a change occurs. Because of that, there's orders of magnitude more space to save by dropping the object column. I think providing both as options would confuse readers who are trying to optimize disk usage.

Do we know if anyone is using save_changes? If it weren't for the plan to have the next release be a major version bump, I'd be interested in adding a deprecation warning that'd ask users to reply on GitHub if they're using the feature.

[seanlinsley]

I updated this PR with a refactor of the serialization code to work around the fact that Rails doesn't provide changes on destroy.

Sometimes the test fails in this way:

  1) PaperTrail record_changes_on_destroy works
     Failure/Error: expect(changes).to eq "id" => [nil, song.id], "length" => [nil, 240]
     
       expected: {"id"=>[nil, 1], "length"=>[nil, 240]}
            got: {"length"=>[nil, 4], "id"=>[nil, 1]}
     
       (compared using ==)
     
       Diff:
       @@ -1,3 +1,3 @@
        "id" => [nil, 1],
       -"length" => [nil, 240],
       +"length" => [nil, 4],

... and sometimes it passes. For me locally, it has always passed. Which doesn't make sense: the test is setting length to 4, then expecting that Paper Trail recorded 240 👻

[seanlinsley]

Ah, the song model has this code in it:

paper_trail/spec/dummy_app/app/models/song.rb

Lines 21 to 31 in 3f9b5be

	class Song < ActiveRecord::Base
	has_paper_trail
	# Uses an integer of seconds to hold the length of the song
	def length=(minutes)
	write_attribute(:length, minutes.to_i * 60)
	end
	def length
	read_attribute(:length) / 60
	end

So we should be expecting a length of 240 to be saved in every case. But with Rails 4.2, a length of 4 is incorrectly being stored.

That might've made sense on the destroy event b/c this PR calls public_send(attr) instead of read_attribute(attr), but the test failure occurs on the create event, not the destroy event.

[jaredbeck]

When trying to save disk space, save_changes is the wrong option .. I think providing both as options would confuse readers who are trying to optimize disk usage.

OK, makes sense. Let's drop save_changes. If someone really needs ultimate control over the object_changes column they can write an object_changes_adapter.

If it weren't for the plan to have the next release be a major version bump, I'd be interested in adding a deprecation warning that'd ask users to reply on GitHub if they're using the feature.

If you want to do that as a separate PR against the 9-stable branch, that'd be great, but I don't want to phrase it as a question. Even if some people are using save_changes, we can still drop it. Like I said above, they can write an object_changes_adapter if they have to.

[seanlinsley]

In that case a deprecation release is probably overkill since there's a clear path if they want the old behavior. Though we'll need to document this in the upgrade guide:

• what are the options, and which should I choose?
• what do I need to do in each case? (with code samples)

We also need to figure out this test failure. AFAICT, it's signalling a bug in Rails 4.2. Should I just update the test to use a different model?

[jaredbeck]

I can try to help with the Song test, but I'm confused about the record_changes_on_destroy config option. You're going to remove that, right?

[seanlinsley]

Yes, sorry, that'd be removed.

[seanlinsley]

Okay this should be complete, code-wise. I also added a commit to remove save_changes.

I'm planning on testing these changes in my application next week. If this ends up getting merged before then, I'll submit another PR with upgrade steps (particularly, a migration to populate data on old destroy events).

[jaredbeck]

I like how you've split this into two commits, but can you please split it into two (new) PRs? I have separate comments for each commit and I think they'll be easier to discuss as separate PRs. Thanks.

[seanlinsley]

Sure. I broke it out into #1130. Now to update this PR...

[jaredbeck]

I'm concerned this changes the order of operations. Previously, object_changes_adapter happened first, before AttributeSerializers::ObjectChangesAttribute. Now it happens after. Was that intentional?

[seanlinsley]

Hmm, that's not how I'm reading it. Previously we had this code:

data[:object_changes] = recordable_object_changes(changes)

changes used to find the notable changes and serialize them. then recordable_object_changes, the last operation, is what called object_changes_adapter.

[jaredbeck]

This method is getting a bit long. I think we can extract some part of this into a new method. Maybe something like

if record_object_changes?
  data[:object_changes] = object_changes
end
# ..

private

# Rails' implementation returns nothing on destroy, so we build our own hash.
def object_changes
  changes = @record.attributes.map { |attr, value| [attr, [value, nil]] }.to_h
  changes = serialize_changes(changes)
  recordable_object_changes(changes)
end

What do you think?

[seanlinsley]

The previous level of abstraction we had in changes clearly wasn't right, since both the update and destroy events would customize the underlying changeset.

Currently every event has these two lines in common w/out any modifications:

changes = serialize_changes(changes)
changes = recordable_object_changes(changes)

What if we introduced a method to wrap those?

# destroy.rb
if record_object_changes?
  changes = @record.attributes.map { |attr, value| [attr, [value, nil]] }.to_h
  data[:object_changes] = prepare_object_changes(changes)
end

# base.rb
def prepare_object_changes(changes)
  recordable_object_changes(serialize_changes(changes))
end

[jaredbeck]

The predominant code style for this repo uses an additional set of parentheses for the eq matcher, and sometimes for the to as well.

expect(changes).to eq("id" => [nil, book.id], "title" => [nil, "A"])

In my experience, using parentheses for neither is hard for many people to follow.

[jaredbeck]

This test looks sufficient, but it'd be great to also have unit tests in eg. spec/paper_trail/events/destroy_spec.rb. Not a requirement, use your judgement.

[seanlinsley]

It doesn't look like the unit tests for the various events currently cover object or object_changes. There's an argument to be made that a lot of the tests in mode_spec.rb should be moved out, but I think that's beyond the scope of this PR. This test is probably enough for now.

[seanlinsley]

I went ahead and refactored this PR. Let me know if there's anything else that needs changing.

[seanlinsley]

🤔

  1) NoObject still creates version records
     Failure/Error: expect(h["created_at"][0]).to be_a(::Time)
       expected "2018-08-13 22:52:16.813300" to be a kind of Time
     # ./spec/models/no_object_spec.rb:33:in `block (2 levels) in <top (required)>'

[jaredbeck]

"Expect to be a kind of Time" is the name of my memoirs.

[jaredbeck]

Thanks for your work on this, Sean.

I'm planning on testing these changes in my application next week.

That'd be great, thanks.

If this ends up getting merged before then, I'll submit another PR with upgrade steps (particularly, a migration to populate data on old destroy events).

How important is such a migration? Should we wait for it before releasing PT 10?

[seanlinsley]

🎉

In the changelog I noted to check #1099 for a backport migration, so a new release doesn't need to wait on the migration.