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

Regression in combination with paper_trail gem #395

Open
AmShaegar13 opened this issue Mar 8, 2022 · 5 comments
Open

Regression in combination with paper_trail gem #395

AmShaegar13 opened this issue Mar 8, 2022 · 5 comments
Assignees

Comments

@AmShaegar13
Copy link

AmShaegar13 commented Mar 8, 2022

Overview

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

f3b33f8

-      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.)

@Laykou
Copy link

Laykou commented May 6, 2022

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

@joshforbes
Copy link

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.

@AmShaegar13
Copy link
Author

New project, same problem. Found my old issue here because I had the same problem again. ClosureTree is not compatible with ActsAsTenant because it uses raw SQL here. I need hierarchy_class to inherit from ActiveRecord::Base in this case to ignore multi tenancy in Hierarchy classes.

@seuros
Copy link
Member

seuros commented May 3, 2024

@AmShaegar13 what versions do you use ?

I have a full rewrite incoming, it will support only 7.x for now.

@AmShaegar13
Copy link
Author

AmShaegar13 commented May 3, 2024

acts_as_tenant (1.0.1)
closure_tree (7.4.0)
rails (7.1.3.2)

The current workaround is to use acts_as_tenant :account in each model instead of ApplicationRecord. Which works, but you could forget about that when adding new models. That's why I intended it to be inherited from ApplicationModel.

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

No branches or pull requests

4 participants