closure_tree icon indicating copy to clipboard operation
closure_tree copied to clipboard

Regression in combination with paper_trail gem

Open AmShaegar13 opened this issue 3 years ago • 5 comments

Overview

Recent changes introduced in v7.4.0 led to a regression in combination with paper_trail. Specifically, this change:

https://github.com/ClosureTree/closure_tree/commit/f3b33f81157ccf002b8c2774b30dc35bd4722610

-      hierarchy_class = parent_class.const_set(short_hierarchy_class_name, Class.new(ActiveRecord::Base))
+      hierarchy_class = parent_class.const_set(short_hierarchy_class_name, Class.new(model_class.superclass))

Description

We need to track changes to each of our models. For this, we use paper_trail in a base model which is then inherited from. Prior to this change, the ModelHierarchy models did not inherit from our base class. Now they do. This activates paper_trail in the ModelHierarchy models which does not work because paper_trail needs a primary key column. The resulting error when creating a new instance of Model is:

ActiveRecord::UnknownPrimaryKey (Unknown primary key for table node_hierarchies in model NodeHierarchy.)

I worked around this by activating paper_trail conditionally only.

# Check if class has a name. Otherwise it is probably a Hierarchy model of closure_tree.
if name.present?
  has_paper_trail
end

However, I wonder if you would like to add an option to use the desired base model with superclassbeing the default. Like:

has_closure_tree hierarchy_base_class: 'ActiveRecord::Base'

Maybe one could even add this globally to the initializer.

Steps to reproduce

I created a simple rails application to demonstrate the problem: Demo App ApplicationRecord (with paper_trail) Node < ApplicationRecord (with closure_tree)

  1. Setup MySQL database in database.yml
  2. rails db:migrate
  3. rails c
  4. Node.create!(name: 'Foo')
  5. ActiveRecord::UnknownPrimaryKey (Unknown primary key for table node_hierarchies in model NodeHierarchy.)

AmShaegar13 avatar Mar 08 '22 10:03 AmShaegar13

This seems like a great idea to be able to customize the base class

Laykou avatar May 06 '22 12:05 Laykou

Similar problem here except with a MultiTenant gem. Basically my group hierarchies now also need a column for the tenant id. It's not a huge deal (I can add that column) but it is a breaking change.

joshforbes avatar Dec 14 '22 19:12 joshforbes