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

Fix belongs_to reify for associations that use a different foreign_key #938

Merged

Conversation

veryeasily
Copy link
Contributor

@veryeasily veryeasily commented Mar 19, 2017

Paper trail currently fails to reify belongs to associations that use a non-standard foreign key. You can see this by spinning up an example rails app:

rails g model Person name:string
rails g model Dog name:string owner_id:integer

# app/models/person.rb
class Person < ApplicationRecord
  has_paper_trail
end

# app/models/dog.rb
class Dog < ApplicationRecord
  has_paper_trail
  belongs_to :person, foreign_key: :owner_id
end

and then creating a few models and attempting to do dog.versions.last.reify(belongs_to: true). You get:

irb(main):002:0> dog.versions.last.reify(belongs_to: true)
  PaperTrail::Version Load (0.2ms)  SELECT  "versions".* FROM "versions" WHERE "versions"."item_id" = ? AND "versions"."item_type" = ? ORDER BY "versions"."created_at" DESC, "versions"."id" DESC LIMIT ?  [["item_id", 1], ["item_type", "Dog"], ["LIMIT", 1]]
  Dog Load (0.1ms)  SELECT  "dogs".* FROM "dogs" WHERE "dogs"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
NoMethodError: undefined method `person_id' for #<Dog:0x005642ccdf5290>

PaperTrail::Reifiers::BelongsTo, I believe, is checking the wrong association method to get its foreign key. It can just check assoc.foreign_key instead of assoc.association_foreign_key which I believe is designed for has_many situations. Happy to provide any other information or do any extra work it takes to get this merged in. Also, I'm not sure if you guys do backports, but my work is currently using paper trail 5, so it would be great to get it merged in there as well if possible!

@jaredbeck
Copy link
Member

Hi Luke, sounds like you're onto something. Please add:

  1. A test. spec/models/person_spec.rb would probably be the easiest place, because Person has a non-standard foreign key already.
  2. A changelog entry under Unreleased -> Fixed.

Also, I'm not sure if you guys do backports, but my work is currently using paper trail 5, so it would be great to get it merged in there as well if possible!

We can probably manage that. Let's get it fixed in master first and then you can open a new PR for the backport, targeting the 5-stable branch.

@jaredbeck jaredbeck self-requested a review March 20, 2017 03:20
@jaredbeck jaredbeck self-assigned this Mar 20, 2017
@jaredbeck jaredbeck removed their request for review March 20, 2017 03:20
@veryeasily
Copy link
Contributor Author

veryeasily commented Mar 26, 2017

Ok! Just added the test and the change log. I ended up needing to add in a foreign key for Person since none of the records appears to have the the type of association I was looking for. Let me know if there's anything you need me to fix!

@veryeasily veryeasily force-pushed the fix-belongs-to-foreign-key branch from 3b7df3b to 48f1194 Compare March 26, 2017 04:39
@jaredbeck jaredbeck merged commit 8a703d6 into paper-trail-gem:master Mar 27, 2017
@jaredbeck
Copy link
Member

Merged, thanks Luke!

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.

2 participants