Fix `joins(:versions)` #1137

seanlinsley · 2018-08-15T16:04:01Z

Fixes #1135 (followup to #1108).

Where should I add a test for my use of STI without a type column? I guess it'll require a new set of model definitions, so it might deserve its own test file, too.

seanlinsley · 2018-08-15T16:18:14Z

spec/models/pet_spec.rb

@@ -112,7 +112,7 @@

    # After creating a bunch of records above, we change the inheritance_column
    # so that we can demonstrate passing hints to the migration generator.
-    context "simulate a historical change to inheritance_column" do
+    xcontext "simulate a historical change to inheritance_column" do


I don't totally understand these tests... not sure how to fix them. Any ideas?

jaredbeck · 2018-08-15T16:27:43Z

I have looked through this, but I didn't understand any of it, probably because I don't understand what problem you're trying to solve. If you could provide a reproduction of the issue using the bug report template that would help a lot.

@lorint can you help, please?

seanlinsley · 2018-08-15T16:31:31Z

@jaredbeck yeah I understand it's a complex issue. I added failing tests as a separate commit so you can see the failures on CI.

seanlinsley · 2018-08-15T16:44:03Z

Interesting that with the fix, the tests only pass on Rails 5.2. I wonder how the API changed 🤔

lorint · 2018-08-16T14:23:15Z

I think I've almost figured out in the has_many :versions lambda how to keep the various tests using .length and .size the way they were, so less overall impact and perhaps less of a breaking change for folks.

-= more news forthcoming =-

seanlinsley · 2018-08-16T14:28:13Z

That's good to hear. In case it wasn't clear, I had to update those tests b/c the events in record_trail.rb were calling versions.build, and that apparently overrode the item_type that was being passed in. As a consequence, the in-memory association wasn't aware of the new record so count was needed to trigger a new database request.

seanlinsley · 2018-08-16T15:50:37Z

lib/paper_trail/model_config.rb

-          unless item_type.nil?
-            relation = relation.unscope(where: :item_type).where(item_type: item_type)
-          end
+          item_type = klass.paper_trail.send(:versions_association_item_type)


it's now a public method, so send isn't required here

Looks like the .unscope / .where can simply happen if klass.descendants.empty?

lorint · 2018-08-17T23:15:56Z

Here's the changes I would recommend:

https://github.com/seanlinsley/paper_trail/pull/1/files

lorint

Been fun to play with this change in the has_many lambda in order to get things going for .joins(). Thanks for bringing it up.

lorint · 2018-08-17T23:17:20Z

lib/paper_trail/model_config.rb

+          super
+          paper_trail.send(:setup_versions_association, klass)
+        end
+      })


Love this idea! Worked out well for the fixes for CelebrityFamily tests and migrations to work.

lorint · 2018-08-17T23:28:07Z

lib/paper_trail/model_config.rb

+      else
+        @model_class.name
+      end
+    end


After playing around for a bit it ended up seeming that we could have just if klass.descendants.empty? in the has_many lambda to get total consistency with existing tests.

I don't believe that'll work for the company / management scenario I described here: #1108 (comment)

Oh -- I thought you had already written the tests you were after. Is this perhaps the goal? If you add in this PR on your branch then it passes BTW:

require "spec_helper" class Management < Customer end RSpec.describe Management, type: :model, versioning: true do it { is_expected.to be_versioned } it "utilises the base_class for STI classes having no type column" do # Customer (and thus Management) have no stored `type` column. expect(Management.inheritance_column).to eq("type") expect(Management.columns.map(&:name)).not_to include("type") customer1 = Customer.create(name: "Cust 1") customer2 = Management.create(name: "Cust 2") # When creating a Management the version refers to Customer: cust_pts = PaperTrail::Version.where(item_type: "Customer") expect(cust_pts.count).to eq(2) # Nothing gets tracked as being Managment: mgmt_pts = PaperTrail::Version.where(item_type: "Management") expect(mgmt_pts.count).to eq(0) end end

Perhaps I'll throw it in the mix for good measure because to me it seems to addresses what sounded like your suggestion. But if this isn't the goal then please write a failing test that addresses Company / Management.

It looks like that test covers the scenario I described; thanks for writing it. Are you sure that it passes when checking descendants.empty?? It'd seem like it'd only pass when we rely on descends_from_active_record?.

Wonderfully enough it works fine with descendants.empty?. Now I've investigated the other override you mentioned, for creation_attributes. It ends up a bit more involved since the patch is a more generic class method that must check to see if it's dealing with a PaperTrail::Version. (The other override was only creating methods directly on Reflection objects.) Pick whichever one you prefer as both approaches tick all of the boxes for the tests.

Ah, interesting. I didn't realize that a monkeypatch on creation_attributes is effectively global, while the collection? monkeypatch is tied specifically to the versions assocation.

I'll go back to the .collection?() override then, which is specific to a Reflection object.

lorint · 2018-08-17T23:30:57Z

lib/paper_trail/record_trail.rb

@@ -84,8 +84,7 @@ def record_create
      # `data_for_create` but PT-AT still does.
      data = event.data.merge(data_for_create)

-      versions_assoc = @record.send(@record.class.versions_association_name)
-      version = versions_assoc.new(data)
+      version = @record.class.paper_trail.version_class.new(data)


Weirdly I couldn't get these guys to fully work unless I did:
version = @record.send(@record.class.versions_association_name).new(data)

lorint · 2018-08-18T10:41:13Z

... events in record_trail.rb ... overrode the item_type that was being passed in. As a consequence, the in-memory association wasn't aware of the new record so count was needed to trigger a new database request.

You describe the bane of my existence. Although this last update in PR #1 might offer an unobtrusive solution to all of this.

jaredbeck · 2018-08-20T17:53:12Z

Should this block the release of PT 10?
Do you need my help reviewing this? If so, is it ready for review?

Thanks.

lorint · 2018-08-20T19:38:52Z

Should this block the release of PT 10?

I would love to see this be a part of PT 10 because it brings together a couple great fixes, and gets us well-prepared to bring the association_tracking gem further.

The fully working version with all the bells and whistles is here, and I feel that it's ready for review.

With that one all existing tests get to stay exactly the same -- except for one simple line added in the dummy app's callback_modifier.rb. It also incorporates the Company / Management STI fix Sean had requested.

jaredbeck · 2018-08-20T21:11:26Z

I would love to see this be a part of PT 10 ..

Fine by me.

The fully working version with all the bells and whistles is here, and I feel that it's ready for review.

So what's the plan? Sean merges that (seanlinsley#1) and then I review it here? Sorry for jumping into the middle of the conversation here.

With that one all existing tests get to stay exactly the same ..

That's the dream 👍

lorint · 2018-08-21T00:10:49Z

the plan -- Sean merges that and then I review it here?

Yeah, that's what I was hoping for. I had originally set up as that PR to his branch instead of a review here with the hope to make it easy to incorporate all the little changes that bring everything together. And if you like what you see out there then it's cool also if you'd like to just make a single commit here that brings all of that in. Whatever is easy.

seanlinsley · 2018-08-21T21:29:46Z

I was planning to merge seanlinsley#1 into this branch, likely as a squash commit.

@jaredbeck we could use more eyes on the monkeypatch part of the PR: seanlinsley#1 (comment)

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

jaredbeck · 2018-08-27T01:20:01Z

I believe this can be closed per #1143, correct me if I'm wrong

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - paper-trail-gem#1135 - paper-trail-gem#1137 - seanlinsley#1

add failing tests for joins bug

3c08782

seanlinsley force-pushed the sti-join-bug branch from 9e8c5a3 to 3c08782 Compare August 15, 2018 16:11

seanlinsley changed the title ~~Fix joins~~ Fix joins(:versions) Aug 15, 2018

seanlinsley commented Aug 15, 2018

View reviewed changes

fix joins(:versions)

e7439b8

seanlinsley force-pushed the sti-join-bug branch from ef18fc3 to e7439b8 Compare August 15, 2018 16:29

seanlinsley mentioned this pull request Aug 15, 2018

Fix for issue #594, reifying sub-classed models that use STI #1108

Merged

seanlinsley commented Aug 16, 2018

View reviewed changes

lorint reviewed Aug 17, 2018

View reviewed changes

seanlinsley mentioned this pull request Aug 21, 2018

Make STI .joins() fix more consistent with existing tests seanlinsley/paper_trail#1

Open

jaredbeck added a commit that referenced this pull request Aug 22, 2018

Revert #1108 (lorint's STI fix)

9f004a6

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

jaredbeck closed this Aug 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `joins(:versions)` #1137

Fix `joins(:versions)` #1137

seanlinsley commented Aug 15, 2018 •

edited

Loading

seanlinsley Aug 15, 2018

jaredbeck commented Aug 15, 2018

seanlinsley commented Aug 15, 2018

seanlinsley commented Aug 15, 2018

lorint commented Aug 16, 2018

seanlinsley commented Aug 16, 2018

seanlinsley Aug 16, 2018

lorint Aug 18, 2018

lorint commented Aug 17, 2018

lorint left a comment

lorint Aug 17, 2018

lorint Aug 17, 2018 •

edited

Loading

seanlinsley Aug 19, 2018

lorint Aug 19, 2018 •

edited

Loading

seanlinsley Aug 19, 2018

lorint Aug 19, 2018 •

edited

Loading

seanlinsley Aug 21, 2018

lorint Aug 22, 2018

lorint Aug 17, 2018

lorint commented Aug 18, 2018 •

edited

Loading

jaredbeck commented Aug 20, 2018

lorint commented Aug 20, 2018

jaredbeck commented Aug 20, 2018

lorint commented Aug 21, 2018

seanlinsley commented Aug 21, 2018

jaredbeck commented Aug 27, 2018

Fix joins(:versions) #1137

Fix joins(:versions) #1137

Conversation

seanlinsley commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

jaredbeck commented Aug 15, 2018

seanlinsley commented Aug 15, 2018

seanlinsley commented Aug 15, 2018

lorint commented Aug 16, 2018

seanlinsley commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint commented Aug 17, 2018

lorint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint commented Aug 18, 2018 • edited Loading

jaredbeck commented Aug 20, 2018

lorint commented Aug 20, 2018

jaredbeck commented Aug 20, 2018

lorint commented Aug 21, 2018

seanlinsley commented Aug 21, 2018

jaredbeck commented Aug 27, 2018

Fix `joins(:versions)` #1137

Fix `joins(:versions)` #1137

seanlinsley commented Aug 15, 2018 •

edited

Loading

lorint Aug 17, 2018 •

edited

Loading

lorint Aug 19, 2018 •

edited

Loading

lorint Aug 19, 2018 •

edited

Loading

lorint commented Aug 18, 2018 •

edited

Loading